Skip to content

feat: add CJS export for cz-commitlint#3963

Merged
escapedcat merged 3 commits intoconventional-changelog:masterfrom
frantic1048:feat/add-cjs-export-for-cz-commitlint
Mar 11, 2024
Merged

feat: add CJS export for cz-commitlint#3963
escapedcat merged 3 commits intoconventional-changelog:masterfrom
frantic1048:feat/add-cjs-export-for-cz-commitlint

Conversation

@frantic1048
Copy link
Copy Markdown
Contributor

@frantic1048 frantic1048 commented Mar 8, 2024 •
edited
Loading

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
@codesandbox-ci
Copy link
Copy Markdown

codesandbox-ci bot commented Mar 8, 2024 •
edited
Loading

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
Merged
10 tasks
JounQin
JounQin suggested changes Mar 8, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

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

commitlint/.eslintrc.cjs

Line 40 in 67baff9

files: ['*.ts'],

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

@frantic1048 frantic1048 requested a review from JounQin March 8, 2024 05:18
JounQin
JounQin reviewed Mar 8, 2024
View reviewed changes
@commitlint/cz-commitlint/src/index.cts Outdated
@@ -0,0 +1,7 @@
const esmPrompterPromise = import('./index.js');
Copy link
Copy Markdown
Collaborator

@JounQin JounQin Mar 8, 2024

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
Copy Markdown
Contributor Author

@frantic1048 frantic1048 Mar 11, 2024

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
Copy Markdown
Contributor

knocte commented Mar 9, 2024

CI is broken

@frantic1048
Copy link
Copy Markdown
Contributor Author

frantic1048 commented Mar 11, 2024

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

@JounQin
Copy link
Copy Markdown
Collaborator

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
@escapedcat
Copy link
Copy Markdown
Member

escapedcat commented Mar 11, 2024

🚀

@frantic1048
Copy link
Copy Markdown
Contributor Author

frantic1048 commented Mar 11, 2024

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/cz-commitlint@19.0.3
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
Copy Markdown
Contributor Author

frantic1048 commented Mar 11, 2024

@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
Copy Markdown
Member

escapedcat commented Mar 11, 2024

Ah sorry :D
Won't release anything for now

@frantic1048 frantic1048 mentioned this pull request Mar 11, 2024
Merged
@JounQin
Copy link
Copy Markdown
Collaborator

JounQin commented Mar 11, 2024

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

@YoutacRandS-VA YoutacRandS-VA mentioned this pull request Jun 20, 2024
Open
This was referenced Sep 12, 2024
Open
Open
@Exkaleburx Exkaleburx mentioned this pull request Sep 22, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@JounQin JounQin JounQin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

4 participants

@frantic1048 @knocte @JounQin @escapedcat