fix(config-conventional): use default export #3911
Conversation
dargmuesli
force-pushed
the
fix/config-conventional/export
branch
from
3ccbe92
to
6eee10d
Compare
| @@ -4,7 +4,7 @@ import { | |||
| TargetCaseType, | |||
| } from '@commitlint/types'; | |||
|
|
|||
| export const config = { | |||
| export default { | |||
There was a problem hiding this comment.
Would something like this also work? To have both? If this makes sense
export const config = {...}
export default config;
There was a problem hiding this comment.
It would prevent introducing another breaking change. 😅
There was a problem hiding this comment.
That should work, I think.
There was a problem hiding this comment.
Hmm, checking it now I don't think that'll work. There can only be one export = which seems to be needed for the fix, unless I don't know of some TypeScript compiler option magic.
There was a problem hiding this comment.
I've added an ESM wrapper which may help in this case.
There was a problem hiding this comment.
Maybe adding tests for:
- CommonJS project using
require() - ESM project using
import
But I do not know how to do it without creating two projects with package.json and so on locally importing the transpiled files from config-conventionnal.
I do not know if node.js project mocking exists in test tools.
There was a problem hiding this comment.
I also had to readd main because some existing test required it.
dargmuesli
force-pushed
the
fix/config-conventional/export
branch
from
6eee10d
to
8b0f791
Compare
dargmuesli
force-pushed
the
fix/config-conventional/export
branch
from
4c6cd2a
to
61fc18b
Compare
|
Can you guys add regression tests so that this doesn't happen again? |
The regression test for default export test was already there and covers already TS use case (which includes direct ESM or CommonJS use case depending on the project package.json "module" field), but the PR causing the regression modified it to "fix" test failures and the PR review fails to detect the related breaking changes. Since it's a major regression introduced in a patch level version release, any enhancements to mitigate human errors at export/import changes for that package with tests should be postponed to later and the priority should be to release a patch level version fixing the regression. |
I agree with this, but you just needed to say that this PR is actually modifying tests too, which I hadn't caught before. So, utACK from me. |
The complete context usually helps at taking wise decision. In its current state, the UTs still not cover all import use cases like import from CommonJS or ESM only projects. But I guess an issue can be created to ensure it will be planned: transpile the TS index. ... test to CommonJS and ESM with the proper file extension and also run them. |
|
Thanks everyone! Exciting times. |
fix bug in 18.6.1 where it was throwing an error on commit Bug Fixes fix(config-conventional): use default export by @dargmuesli conventional-changelog/commitlint#3911 Signed-off-by: Florent Benoit <fbenoit@redhat.com>
fix bug in 18.6.1 where it was throwing an error on commit Bug Fixes fix(config-conventional): use default export by @dargmuesli conventional-changelog/commitlint#3911 Signed-off-by: Florent Benoit <fbenoit@redhat.com>
Description
Resolves #3909
Motivation and Context
Reverts the new
configexport.Usage examples
n/a
How Has This Been Tested?
locally
Types of changes
Checklist: