-
Notifications
You must be signed in to change notification settings - Fork 914
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(load): Remove dependency on ts-node for @commitlint/load #3633
feat(load): Remove dependency on ts-node for @commitlint/load #3633
Conversation
…pendency for @commitlint/load
Thanks for this @joberstein ! Great PR! 🎉 |
Thanks @escapedcat and for merging so quickly! Is there a particular cadence for releases? And it seems like it happens manually based on the README, is that right? |
Yeah, it's manually. So either rolling prettier back to v3 or updating to lerna v7 it is. Haven't decided yet. But that's the current status :P |
Got it, thanks! It's not urgent or anything at least. |
Decided to roll prettier back: https://github.com/conventional-changelog/commitlint/releases/tag/v17.7.0 |
@escapedcat I'm a bit confused, that release reverted this PR (which I'm really waiting for) rather than reverting the prettier changes. From your comment it sounds like this PR was meant to stay in the release? |
The rollback was actually for a different reason; the conversation about prettier and lerna was in regards to blocking this change from releasing. Once it was released, users who were automatically installing (with semver latest minor version syntax) this package with node 12/14 were facing an issue because I didn't release this as a breaking change, and one of libraries I upgraded only works with node 16+. As a result, this change was reverted, but I think it can be re-released soon once the project supports node 16 at minimum (there's an open PR ready to merge for this). |
@joberstein #3644 is merged and released. Wanna try again? |
@escapedcat sure, should I just re-apply my changes? It looks like the checks failed for: #3654 though. |
Yeah, I tried to fix it but also failed. Let's leave it out for now. |
Hi everyone, I haven't contributed to a public repo before and don't have push access, so not sure I've done this right, but I've forked off the original repo and push a branch.
Upgrades the cosmiconfig-typescript-loader dependency in @commitlint/load to move away from a production dependency on ts-node.
Description
Prior to cosmiconfig-typescript-loader@5, ts-node was used to transpile Typescript files, which required it and typescript to be declared as production dependencies in @commitlint/load. In v5, cosmiconfig switched to using https://www.npmjs.com/package/jiti in place of ts-node (https://github.com/Codex-/cosmiconfig-typescript-loader/releases/tag/5.0.0).
As a result, I have removed the dependency on ts-node, and cleaned up the dependencies for @commitlint/load - removed unused dependencies and moved typescript to the dev dependencies.
Motivation and Context
I initially wanted to make this change because 'ts-node' was causing issues when I tried to bundle @commitlint/load in a project I'm working on. With the move away from 'ts-node' as dependency, I was successfully able to bundle a project using @vercel/ncc when including @commitlint/load as a dependency.
I believe this change will also resolve at minimum the following issues:
Usage examples
N/A - no change in usage
How Has This Been Tested?
Locally, I ran:
yarn build
in the top-level of the reponpm link @commitlint/load
in my dependent projectTypes of changes
Not sure if this counts as a bug fix or a new feature since it's a dependency major version bump, but no API changes.
Checklist: