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

Add Node-specific export condition for ESM entrypoint that re-exports CJS #201

Merged
merged 2 commits into from
May 1, 2023

Conversation

andrewbranch
Copy link
Member

Another stab at #171 / #143. Ensures non-Node module resolvers that use import do not see CommonJS, while still ensuring Node only ever sees one copy of the helpers.

Copy link
Member

@rbuckton rbuckton left a comment

Choose a reason for hiding this comment

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

As far as I can tell, this seems fine.

@andrewbranch
Copy link
Member Author

@weswigham can you take a look at this one too?

},
"import": {
"node": "./modules/index.js",
"default": {
Copy link
Member

Choose a reason for hiding this comment

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

This is relying on microsoft/TypeScript#50762 to resolve types for the node import case, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

It’s going to use #200

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

Assuming the plan is to add another types entry for the import node case, this seems OK. Though.... what currently resolves export maps without the node condition (but with the import condition) anyway? Might just be good to have an idea of what runtimes that branch is actually for.

@andrewbranch
Copy link
Member Author

andrewbranch commented May 1, 2023

I also want to know this. I (indirectly) asked @justinfagnani at #171 (comment).

@andrewbranch andrewbranch merged commit 7def846 into main May 1, 2023
@andrewbranch andrewbranch deleted the fix-esm branch May 1, 2023 21:30
@Andarist
Copy link
Contributor

Andarist commented May 2, 2023

Though.... what currently resolves export maps without the node condition (but with the import condition) anyway?

Bundler configurations targeting browsers could do that. For example, Rollup doesn't include node by default:
https://github.com/rollup/plugins/blob/83197203e8fb506851abec0706666502b7ae91a8/packages/node-resolve/src/index.js#L30-L32

It also doesn't even include browser by default - that's an opt-in:
https://github.com/rollup/plugins/blob/83197203e8fb506851abec0706666502b7ae91a8/packages/node-resolve/src/index.js#L162-L163

@andrewbranch
Copy link
Member Author

Oh yeah, sure, but bundlers are all capable of running into CJS files too. I wanted to know who’s doing this and doesn’t support CJS modules at all.

@SimenB
Copy link
Contributor

SimenB commented May 31, 2023

FWIW, this has caused a regression in Jest jestjs/jest#14173

From my understanding, when given import and browser (i.e. no node) condition, you now return a js file in root, where the package.json does not contain type: module, which causes Jest to load the file as CJS.

FWIW, Jest follows Node: https://nodejs.org/api/packages.html#packagejson-and-file-extensions

Within a package, the package.json "type" field defines how Node.js should interpret .js files. If a package.json file does not have a "type" field, .js files are treated as CommonJS.

@Andarist
Copy link
Contributor

Andarist commented May 31, 2023

where the package.json does not contain type: module, which causes Jest to load the file as CJS.

This sounds like a bug in Jest. type: module is correctly set in the nearest package.json of that .js file:
https://github.com/microsoft/tslib/blob/e623061dc031172d9e5075bdba120f4c61bd3eeb/modules/package.json

Perhaps you are only looking into the root package.json file in your resolver?

@SimenB
Copy link
Contributor

SimenB commented May 31, 2023

It resolves to tslib.es6.js in root, not within modules directory. Which is the correct thing to do when we don't provide the node condition.

What might be wrong is applying node semantics to .js when we don't provide the node condition. But Jest has been doing that ever since exports support was added months and months ago. And I'm not sure how else to know how to treat a .js file.

Note that [email protected] works correctly as Jest indeed looks at modules/package.json.

@Andarist
Copy link
Contributor

It resolves to tslib.es6.js in root, not within modules directory. Which is the correct thing to do when we don't provide the node condition.

Ah, sorry - I missed that this was about requests without the node condition.

What might be wrong is applying node semantics to .js when we don't provide the node condition

It's kinda hard to tell for me on the spot - since that's usually bundlers' territory and they tend to do different interop things. @andrewbranch prepared a comprehensive table that showcases a lot of things (here) but I have a hard time unwrapping this quickly to find the positions related to your specific situation.

What is easy to test is the node's behavior and this one just throws a SyntaxError when trying to import a CJS file like that. My own intuition is that this should be OK (preferred). OTOH, this has been structured like this for a reason. Maybe the option here would be to:

  • keep doing what it is doing for node (reexport CJS through an ESM wrapper)
  • use ESM file for other runtimes (use ESM file directly, just make sure that this .js gets interpreted as a module, just in case)

This seems quite reasonable to me as that would only alter this .js's "type" and not much else so it shouldn't quite have a negative effect on much.

It's probably best to wait on Andrew's opinion though.

@andrewbranch
Copy link
Member Author

On principle, I agree it’s sketchy at best to ship a .js file with ESM syntax and no "type": "module". But I’m pretty sure someone else had a reason that some other tool would break if we renamed tslib.es6.js to tslib.es6.mjs or even moved it into a directory. I think there was concern that people are mapping to specific file locations within the package. There’s simply no way to fix everything without breaking someone’s edge case.

Can Jest handle CJS format files at all? If so, why is it setting the browser condition?

@andrewbranch
Copy link
Member Author

Thinking about duplicating tslib.es6.js to tslib.es6.mjs and only exposing the latter through the package.json exports

@SimenB
Copy link
Contributor

SimenB commented May 31, 2023

Can Jest handle CJS format files at all?

Yes, that's the default - ESM is opt in (since Node's vm APIs are flagged).

If so, why is it setting the browser condition?

It's a browser-like environment (jsdom). So global APIs are the ones found in a browser, but the module system is separate from that.

@josundt
Copy link

josundt commented Jun 2, 2023

tslib's package.json exports are now ambigous.

a. All Files under the import conditional export object imply that they are esm modules thorugh the fact that they are listed under import.
b. Still the file ./tslib.es6.js imply that it is a commonjs module by the fact that the folder's package.json file has no type property which implies "commonjs" when the file does not have a .mjs file extension.

tslib's exports should ideally not have this ambiguity.
When it is ambiguous like this; which of the two facts should take precedence?

I notice that with webpack's enhanced-resolve which has no issues with [email protected], a. seemingly takes precedence over b..
With jest's resolver, which has issues with [email protected], b. takes precedence over a..

Are you planning to do anything about the exports' ambiguity in tslib?

@Andarist
Copy link
Contributor

Andarist commented Jun 2, 2023

When it is ambiguous like this; which of the two facts should take precedence?

IIUC, the import condition doesn't exactly imply that the resolved file will be interpreted as a module. This is just a condition used for all import "requests" but has nothing to say about the format of the resolved file. But ofc, it usually only makes sense to have parity between those 2 things.

@josundt
Copy link

josundt commented Jun 2, 2023

@Andarist Then I guess the exports of tslib are not ambigous but rather incorrect?

@Andarist
Copy link
Contributor

Andarist commented Jun 2, 2023

To quote @andrewbranch:

On principle, I agree it’s sketchy at best to ship a .js file with ESM syntax and no "type": "module"

If we assume that the node's behavior is the correct one here that guides everything else... then ye, I'd classify this as incorrect. Different tools implement things slightly differently though so at times it's super hard to determine how to solve things to satisfy everybody though. I think that "default" should still use node's semantics - even if the node condition itself stays unmatched for a given request. It feels that's the only way to actually ensure compatibility with the ecosystem. If some tool couldn't deal with a specific situation that was compatible with node's semantics given a particular exports shape then I'd prefer to raise that issue with that tool and try to fix it there.

@SimenB
Copy link
Contributor

SimenB commented Jun 2, 2023

I notice that with webpack's enhanced-resolve which has no issues with [email protected], a. seemingly takes precedence over b..
With jest's resolver, which has issues with [email protected], b. takes precedence over a..

enhanced-resolve behaves the same way as Jest does when provided the conditions.

// file.js
const path = require('node:path');
const enhancedResolve = require('enhanced-resolve');

const resolver = enhancedResolve.create({
  // this is the important bit
  conditionNames: ['browser', 'import'],
});

console.log(require('tslib/package.json').version);

const cwd = process.cwd();

resolver(cwd, 'tslib', (err, res) => {
  if (err) {
    console.error('got an error', err);
  } else {
    console.log(path.relative(cwd, res));
  }
});
$ node file.js
2.5.2
node_modules/tslib/tslib.es6.js

I think that "default" should still use node's semantics - even if the node condition itself stays unmatched for a given request.

FWIW, I agree with this. The exports spec comes from Node, so when there's ambiguity, I think their semantics are reasonable to follow. But I know not everyone agrees with this. 😅

@andrewbranch
Copy link
Member Author

I was hoping to get feedback on this

Thinking about duplicating tslib.es6.js to tslib.es6.mjs and only exposing the latter through the package.json exports

@SimenB
Copy link
Contributor

SimenB commented Jun 2, 2023

That should work fine, and no drawbacks as far as I know

@andrewbranch
Copy link
Member Author

The irony of exporting __importStar and __importDefault from an ES module... 🙃

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

Successfully merging this pull request may close these issues.

6 participants