Skip to content
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

Conversation

joberstein
Copy link
Contributor

@joberstein joberstein commented Aug 8, 2023

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 repo
  • (cd @commitlint/load && npm link) to link the package
  • Ran npm link @commitlint/load in my dependent project
  • Verified the node_modules in my dependent project had the updated package.json
  • Re-built my dependent project
  • Ran my dependent project and verified the commitlint config loaded.

Types 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.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation. (n/a)
  • I have updated the documentation accordingly. (n/a)
  • I have added tests to cover my changes. (n/a)
  • All new and existing tests passed. (or at least nothing new failed)

@joberstein joberstein changed the title feat(load): use cosmiconfig-typescript-loader v5 to remove ts-node dependency for @commitlint/load feat(load): Remove dependency on ts-node for @commitlint/load Aug 8, 2023
@escapedcat
Copy link
Member

so not sure I've done this right

Thanks for this @joberstein ! Great PR! 🎉

@escapedcat escapedcat merged commit 4aa46d7 into conventional-changelog:master Aug 8, 2023
5 checks passed
@joberstein
Copy link
Contributor Author

so not sure I've done this right

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?

@escapedcat
Copy link
Member

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.
Wanted to release earlier but forgot about the incompatibility of prettier v3 and lerna v6. And recently I just merged prettier v3 again (because I forgot about the incompatibility) and now can't release again.

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
Kinda would like to update lerna but need to look into it. If you need this timely rolling back prettier might be faster.

@joberstein
Copy link
Contributor Author

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. Wanted to release earlier but forgot about the incompatibility of prettier v3 and lerna v6. And recently I just merged prettier v3 again (because I forgot about the incompatibility) and now can't release again.

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 Kinda would like to update lerna but need to look into it. If you need this timely rolling back prettier might be faster.

Got it, thanks! It's not urgent or anything at least.

@escapedcat
Copy link
Member

Decided to roll prettier back: https://github.com/conventional-changelog/commitlint/releases/tag/v17.7.0

escapedcat added a commit that referenced this pull request Aug 10, 2023
…-node dependency for @commitlint/load (#3633)"

This reverts commit 4aa46d7.
@vlindhol
Copy link

vlindhol commented Oct 1, 2023

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?

@joberstein
Copy link
Contributor Author

joberstein commented Oct 1, 2023

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).

@escapedcat
Copy link
Member

@joberstein #3644 is merged and released. Wanna try again?
#3654 is still running checks currently but maybe it can be added as well.

@joberstein
Copy link
Contributor Author

@escapedcat sure, should I just re-apply my changes? It looks like the checks failed for: #3654 though.

@joberstein joberstein deleted the feat/bump-cosmiconfig-typescript-loader branch October 24, 2023 02:21
@joberstein joberstein restored the feat/bump-cosmiconfig-typescript-loader branch October 24, 2023 02:22
@escapedcat
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants