Closed andrewbranch closed 1 year ago
@weswigham can you take a look at this one too?
I also want to know this. I (indirectly) asked @justinfagnani at https://github.com/microsoft/tslib/pull/171#issuecomment-1530066148.
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
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.
FWIW, this has caused a regression in Jest https://github.com/jestjs/jest/issues/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 apackage.json
file does not have a"type"
field,.js
files are treated asCommonJS
.
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?
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 tslib@2.5.0
works correctly as Jest indeed looks at modules/package.json
.
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:
node
(reexport CJS through an ESM wrapper).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.
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?
Thinking about duplicating tslib.es6.js
to tslib.es6.mjs
and only exposing the latter through the package.json exports
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.
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 tslib@2.5.2, a.
seemingly takes precedence over b.
.
With jest's resolver, which has issues with tslib@2.5.2, b.
takes precedence over a.
.
Are you planning to do anything about the exports' ambiguity in tslib?
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.
@Andarist Then I guess the exports of tslib are not ambigous but rather incorrect?
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.
I notice that with webpack's
enhanced-resolve
which has no issues with tslib@2.5.2,a.
seemingly takes precedence overb.
. With jest's resolver, which has issues with tslib@2.5.2,b.
takes precedence overa.
.
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. 😅
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
That should work fine, and no drawbacks as far as I know
The irony of exporting __importStar
and __importDefault
from an ES module... 🙃
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.