feat: add support for module-sync export condition in Node.js 22+ #534
Conversation
test/unit.test.js
Outdated
|
|
||
| it(`should correctly trace ${testSuffix}`, async () => { | ||
| // Mock Node.js version for module-sync-condition tests | ||
| if (testName === 'module-sync-condition') { |
There was a problem hiding this comment.
Instead of mocking, lets just skip for older versions of node since we have a ci matrix already.
See the continue statements above.
There was a problem hiding this comment.
Now that I think about it, we need this first:
There was a problem hiding this comment.
Yep just sending a message that CI here doesn't run 22
src/resolve-dependency.ts
Outdated
| (condition === 'import' && !cjsResolve) || | ||
| (condition === 'module-sync' && | ||
| getNodeMajorVersion() >= 22 && | ||
| !cjsResolve) || |
There was a problem hiding this comment.
Are you sure about the !cjsResolve here?
The docs say
"module-sync" - matches no matter the package is loaded via import, import() or require()
There was a problem hiding this comment.
I added because of the what's next come in the doc
The format is expected to be ES modules
I might be wrong though
There was a problem hiding this comment.
Thats the format of the imported module and not the format of the module doing the importing, right?
| @@ -0,0 +1,11 @@ | |||
| { | |||
| "name": "test-pkg-sync-fallback", | |||
| "type": "module", | |||
There was a problem hiding this comment.
Lets add another test with "type": "commonjs" to see what happens
There was a problem hiding this comment.
It passes with commonjs with/without cjeResolve check, I am a little bit confused, probably safe to remove the check?
There was a problem hiding this comment.
Okay tested again, tests are failing with cjs but it really doesn't need the check
There was a problem hiding this comment.
I can't remember if cjeResolve is implying the current module was parsed as cjs or if its the module being imported. Thats why it might need to be unconditional.
Regardless, we should have a test for both types since that works with Node.js as far as I can tell.
There was a problem hiding this comment.
Yes added, as soon as you merge your PR I will rebase and update
hamidrezahanafi
force-pushed
the
fixmodulesyncnode22
branch
from
8cd1179 to
d5740c7
Compare
|
🎉 This PR is included in version 0.30.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR implements support for the module-sync export condition in package.json exports, which is available in Node.js 22 and later versions. The feature enables packages to provide synchronous ES module loading behavior for enhanced performance in supported Node.js versions.
Unit tests added with
process.versions.nodemocked