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: add CJS export for cz-commitlint #3963

Merged
merged 3 commits into from
Mar 11, 2024
Merged

feat: add CJS export for cz-commitlint #3963

merged 3 commits into from
Mar 11, 2024

Conversation

frantic1048
Copy link
Contributor

@frantic1048 frantic1048 commented Mar 8, 2024

resolves #3949

Description

Motivation and Context

Currently comittizen does not support ESM adapter, this PR add additional CJS export for @commitlint/cz-commitlint, which allows comittizen to work with it.

#3850 (comment)

Usage examples

This PR does not change usage.

How Has This Been Tested?

package.json

{
  "config": {
    "commitizen": {
      "path": "@commitlint/cz-commitlint"
    }
  },
}

Install new @commitlint/cz-commitlint and run commitizen:

pnpm add -D https://pkg.csb.dev/conventional-changelog/commitlint/commit/a6401ba0/@commitlint/cz-commitlint
pnpm exec cz # works

Types of 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.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@frantic1048 frantic1048 marked this pull request as draft March 8, 2024 03:06
Copy link

codesandbox-ci bot commented Mar 8, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@frantic1048 frantic1048 marked this pull request as ready for review March 8, 2024 03:25
@frantic1048 frantic1048 mentioned this pull request Mar 8, 2024
10 tasks
Copy link
Contributor

@JounQin JounQin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

files: ['*.ts'],

This line should change to files: ['*.cts', '*.ts'],

@frantic1048 frantic1048 requested a review from JounQin March 8, 2024 05:18
@@ -0,0 +1,7 @@
const esmPrompterPromise = import('./index.js');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking to use typeof import('./index.js') but I found there was a TypeScript error (I believe it's a bug), but considering this, I'm thinking to use <packageDir>/index.cjs which is plain js instead.

Basically it should be await import('./lib/index.js')

@frantic1048 How do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's weird and I'm trying to use cjs to workaround this.

I was thinking to use typeof import('./index.js') but I found there was a TypeScript error

@knocte
Copy link
Contributor

knocte commented Mar 9, 2024

CI is broken

@frantic1048
Copy link
Contributor Author

The error from the tests appears to be related to vitest-dev/vitest#3987 🤔

@JounQin
Copy link
Contributor

JounQin commented Mar 11, 2024

@frantic1048

We can bypass that issue easily.

#3963 (comment)

@escapedcat escapedcat merged commit 6ae3c6a into conventional-changelog:master Mar 11, 2024
7 checks passed
@escapedcat
Copy link
Member

🚀

@frantic1048
Copy link
Contributor Author

Checking the outupt: https://ci.codesandbox.io/status/conventional-changelog/commitlint/pr/3963/builds/482116, it appears the build is not containing index.cjs in output, the build related config may required to be updated to properly pack the package.

npm notice package: @commitlint/[email protected]
npm notice === Tarball Contents === 
npm notice 1.6kB README.md                                  
npm notice 464B  lib/index.d.ts                             
npm notice 372B  lib/index.d.ts.map                         
npm notice 451B  lib/index.js                               
npm notice 407B  lib/index.js.map                           
npm notice 335B  lib/Process.d.ts                           
npm notice 390B  lib/Process.d.ts.map                       
npm notice 948B  lib/Process.js                             
npm notice 965B  lib/Process.js.map                         

@frantic1048
Copy link
Contributor Author

@escapedcat, oh, the merge may have been too hasty. I'm testing the built package and have found an issue with the build configuration. 😮

@escapedcat
Copy link
Member

Ah sorry :D
Won't release anything for now

@JounQin
Copy link
Contributor

JounQin commented Mar 11, 2024

I meant to have .cjs at <packageDir>/index.cjs, and add it into files field in package.json.

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.

Can not use @commitlint/cz-commitlint as commitizen's adapter
4 participants