-
-
Notifications
You must be signed in to change notification settings - Fork 16
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
Do not add default node
, browser
, require
or import
conditions
#12
Comments
Hey, sorry I missed you over in the PR! I kept clicking the notification, but GitHub never showed me anything except the top of the PR file, so I assumed it was was previous mentions from the other PR(s) that got merged into that one. Ok, so I'm willing to work with you on this to see if there's something that can be done here, but there are a few things/misunderstands to correct first before we can figure out any next steps:
Overall, the entire Resolving exports is a two-way street – package authors play an equally-important role as the resolving/consuming side. No one knows the package better than the author(s), so it's up to them to define the priority list. I like to think of it as a recommendation system, where the author(s) have all the products sorted by most to least specific. Consumer walks in and says "I like browser, worker, production, and import." and the author/sommelier says "Perfect! I have just the thing for you: ./worker/index.prod.mjs" "exports": {
"worker": {
"import": {
"production": "./worker/index.prod.mjs",
"development": "./worker/index.dev.mjs",
"default": "./worker/index.prod.mjs"
},
"require": {
"production": "./worker/index.prod.js",
"development": "./worker/index.dev.js",
"default": "./worker/index.prod.js"
}
},
"browser": {
"import": "./browser/index.mjs",
"require": "./browser/index.js"
},
// ...
} |
Hi! Thanks for the response. I'm currently on the move so I just skimmed through, but this jumped out
Even node's examples doesn't have this, so I'm not sure this is true? This is the first example in the docs: https://nodejs.org/api/packages.html#packages_conditional_exports // package.json
{
"main": "./main-require.cjs",
"exports": {
"import": "./main-module.js",
"require": "./main-require.cjs"
},
"type": "module"
} Might just be to keep the examples succinct, I dunno. But if this is a requirement by the spec we should probably open up an issue with node for them to fix their examples.
I agree 100%! Which is why I wanna utilize a module written strictly to the spec rather than implementing our own which sorta works but misses all the edge cases and ends up behaving wrong when running in a "real" env 🙂 I'll read through you response thoroughly tomorrow and properly digest it 👍 |
Indeed, For |
According to the "worker": {
"import": {
"production": "./worker/index.prod.mjs",
"development": "./worker/index.dev.mjs",
"import": "./worker/index.prod.mjs" // <- echo "import" instead of "default"
},
// ...
} See Resolver Algorithm Specification, under
That's because the example has // package.json
{
"main": "./main-require.cjs",
"exports": {
"import": "./main-module.js",
"require": "./main-require.cjs",
"default": "./main-module.js" // < ADDED: but will NEVER match
},
"type": "module"
}
This is true for all conditions. user conditions are a set of options, and the key order specified in the "exports" object determines the order that the resolver checks/determines a match. This is the bare minimum implementation of how "exports" conditions are traversed and nested: // NOTE: `level` is only for logging
function resolve(input, conditions, level = 0) {
let allows = new Set(conditions);
allows.add('default');
let spacer = ' '.repeat(level * 4);
for (let key in input) {
console.log('%s [%s] look', spacer, key);
if (allows.has(key)) {
console.log('%s [%s] KNOWN', spacer, key);
let tmp = input[key];
let type = typeof tmp;
if (type === 'string') {
console.log('%s [%s] DONE', spacer, key, tmp);
return tmp;
}
if (tmp && type === 'object') {
console.log('%s [%s] NESTED', spacer, key);
return resolve(tmp, conditions, level + 1);
}
return console.log('%s [%s] INVALID VALUE', spacer, key, tmp);
} else {
console.log('%s [%s] ignore', spacer, key);
}
}
} It's a simple Here are some examples (warning: wall of text): // Example "exports" object
// All `resolve` examples will use this as input:
let example = {
worker: {
import: "./worker.mjs",
require: "./worker.js"
},
browser: {
import: {
production: "./browser.prod.mjs",
development: "./browser.dev.mjs",
default: "./browser.prod.mjs"
},
require: {
production: "./browser.prod.js",
development: "./browser.dev.js",
default: "./browser.prod.js"
}
},
import: "./fallback.mjs",
require: "./fallback.js"
};
// Find file when supporting "browser" and "import" conditions
// ... and default, implicitly/by default
resolve(example, ['browser', 'import']);
//=> [worker] look
//=> [worker] ignore
//=> [browser] look
//=> [browser] KNOWN
//=> [browser] NESTED
//=> [import] look
//=> [import] KNOWN
//=> [import] NESTED
//=> [production] look
//=> [production] ignore
//=> [development] look
//=> [development] ignore
//=> [default] look
//=> [default] KNOWN
//=> [default] DONE ./browser.prod.mjs
resolve(example, ['browser', 'development', 'import']);
//=> [worker] look
//=> [worker] ignore
//=> [browser] look
//=> [browser] KNOWN
//=> [browser] NESTED
//=> [import] look
//=> [import] KNOWN
//=> [import] NESTED
//=> [production] look
//=> [production] ignore
//=> [development] look
//=> [development] KNOWN
//=> [development] DONE ./browser.dev.mjs
resolve(example, ['worker', 'require']);
//=> [worker] look
//=> [worker] KNOWN
//=> [worker] NESTED
//=> [import] look
//=> [import] ignore
//=> [require] look
//=> [require] KNOWN
//=> [require] DONE ./worker.js
resolve(example, ['require']);
//=> [worker] look
//=> [worker] ignore
//=> [browser] look
//=> [browser] ignore
//=> [import] look
//=> [import] ignore
//=> [require] look
//=> [require] KNOWN
//=> [require] DONE ./fallback.js
resolve(example, ['foobar']);
//=> [worker] look
//=> [worker] ignore
//=> [browser] look
//=> [browser] ignore
//=> [import] look
//=> [import] ignore
//=> [require] look
//=> [require] ignore
// (returns undefined) I have a lot of tests in this repo that are hopefully intelligible to fresh eyes. The core approach is correct and the default conditions match Node's. The algorithm actually works quite well and is very flexible/accommodating, but it does require authors to play their part too. It's the only way we can dream to have a multi-environment resolution standard, which ESM allows & is begging for. |
Note that the import/require/default example will match default in node versions that lack the import and require conditions. |
No because |
@lukeed that's not the case in older versions of node, which don't understand |
That's the case in this lib (and in Node) but should it be true for Require.js, system.js, amd, umd etc are all (imo) valid targets for an To circle back to the OP - I don't necessarily have any objections to this module being an implementation of Node's algorithm, but in that case it should be clear that's the case (and that (Written on mobile, please excuse terseness/typos) |
@ljharb and those versions are early iterations and no longer the resolution algorithm. They've been thrown out. They're also non-LTS versions of Node, which should not be built to or catered towards. And this version range, specifically, has caused so much adoption headache. I personally wish they'd see patch fixes that conformed them to the finalized behavior. It's a wound that's still open. In fact, I see a lot of people specifying this version range in their @SimenB If tools want adopt the exports practice, then they should follow its design or make its own thing. Not ruin/muddy an existing design. The Node resolution algorithm is adequately flexible and can handle all use cases -- I was slightly skeptical of it at first too but it's proven to be a very valuable feature when publishing packages for use across Node, browsers, and Cloudflare Workers. It's been extremely successful for well over a year now. And you're right, the time was taken to design a flexible system (thank you Node 13-13.7 for the early failures). The Node-isms are very very minimal and already, as part of the design, has an easy way to define and prioritize entries ahead of what Node would look for. |
I agree, but I would hope that |
I'm saying it does target non-node environments. That's the whole purpose of user conditions. The only thing to be aware of, as an author, is that the order of your defined conditions determines the priority of the condition itself. I, personally, have used the same module across browser, workers, node, and electron – each with their own entry files – and all through defining the conditions in my package. It's very well-designed, it only suffers from a lack of education / clarity. |
@lukeed i absolutely agree they should all get patch fixes, but node has declined to do so. whether they "should" be supported is each author's choice; every package i maintain will continue to work with every possible node minor for as long as I maintain them :-) It sounds like as long as a |
Node 13 is a dead version – and literally no one has any reason to continue using it. I do not say this as live-on-the-cutting-edge type of person. I generally hold onto support for LTS versions of Node longer than the average developer, but odd-number versions are purposefully experimental. Node itself does not recommend that any one use odd versions in production or beyond the next even-version's release. I'm sure you're familiar with this, so I'm not sure why you are trying to advocate for this idea? Even Node 14 enters "maintenance" in 7 days. Anyone on Node 9/11/13 deserves a bad experience at this point.
Yes, I've laid out a number of examples explaining this. If you only have time to skim one, I'd recommend you look at the output for the The |
In a private conversation, it was confirmed to me that TypeScript is knowingly not conforming to the spec (currently, maybe there's still hope?) and are not considering key-order in their resolution. I very much disagree with this but who am I to say |
Did you get any info (links, preferably) as to why? |
No, just that TS already adjusts/ignores specs when loading anything (eg, .ts preference over .js) so that this isn't outside its wheelhouse. That's not a good reason to me -- and overall this move is going to add further confusion wrt "exports" and how it works/is supposed to be. A bit frustrating since the example/approach would work perfectly fine -- even with their non-compliant loader -- if it were this: "exports": {
"types": "./types/index.d.ts",
"import": "./esm/index.js",
"require": "./commonjs/index.cjs"
} Because then it'd actually work with spec-compliant resolvers, and if they chose to continue ignoring the spec within their own resolver, their resolver would still work too. Any spec-compliant revolver that wasn't looking for "types" condition would safely ignore it. |
Right, I think I'm starting to see where you're coming from. I'm not saying that the way this module works today makes integrating into other runtimes/hosts/resolvers impossible, it just places the burden onto package authors whose packages are used in these envs to always put And even if those conditions were put last, if my runtime doesn't support I wouldn't expect package authors to support my custom runtime, but I would then want to surface that to my users ("package foo doesn't support this runtime") rather than fail at runtime with a syntax error or a Anyways, to be less abstract, I can focus on my own use case, which is Jest. The current approach in this module probably works, since the environments we do support are
It's sorta weird that if I pass So to wrap up - I have nothing against 100% respecting key order (and I find it weird TS pretends they don't matter), my only issue is default conditions I might not support being resolved (which I guess would be undefined behavior based on the runtime its loaded in) instead of "this package doesn't support any of your conditions". |
As mentioned, this is a two-way street. Package authors are required to have some awareness of their exports definition. But you're right, more often than not you'll be defining the
Here's probably my most complex public example;
You'd only ever get a result/match for something you said you supported and/or wanted. Aka, in the httpie example, no one will ever get the "worker" file unless it was explicitly allowed. That's the purpose/safety of custom conditions -- author offers standard X conditions but also offers custom, optional Y conditions (worker, react-native, electron... cough types cough lol) that relevant users can opt into since theyre the target. As an author, if you define this: "exports": {
".": {
"foobar": "./foobar.mjs"
}
} No one will ever see that file unless they've enabled the "foobar" condition (both with this module and with Node natively). Instead, the root import will resolve to nothing/undefined by default, which surfaces as However, doing this: "exports": {
".": {
"foobar": "./foobar.mjs",
"default": "./fallback.js"
}
} ...will resolve to the "fallback.js" file by default for this module and Node, avoiding the undefined/Error. If you really need me to (99.9999% you don't though), I can add an I'd want to keep the name ugly/scary because it's purposefully breaking out of defined behavior. Again, this really should not be needed by anybody who's actually willing to use/work with the exports field. |
I'm very much down with a "exports": {
".": {
"foobar": "./foobar.mjs",
"node": "./node.mjs",
"require": "./common.cjs",
"import": "./import.mjs",
"default": "./fallback.js",
}
} that would never give me Or if I were to remove the |
According to the spec, if a "default" condition is defined, you will never get undefined. In a Node setting, (options.browser remains falsely) then you will always get "node" to match. That's because the author defined it that way. Because it's evaluated before "require"/"import" -- and matches -- then that means this "node" value will always be used, regardless of Now, if it's a browser environment (options.browser = true), then "node" is disabled internally and the resolver is finally able to move beyond that "node" condition. If If, for some reason, you pass So ya, you'll never get to the "default" key in this example (according to spec) because you're ONLY able to access a file thru Sorry for long-winded answer but hopefully that helps |
Ah good point, I forgot
Right, and I think that's partly where my issue comes from - what if it's not https://nodejs.org/api/packages.html#packages_conditional_exports only says "node supports these conditions" and https://nodejs.org/api/esm.html#esm_resolver_algorithm talks specifically about how node resolves ESM (and which default conditions node provides), it doesn't say "this is the way all ESM should be resolved". However, purely from Jest's perspective (which only has "exports": {
".": {
"node": "./node.mjs",
"browser": "./browser.cjs",
"default": "./fallback.js"
}
}
It definitely does, thanks for being patient with me! |
@SimenB I gotta run, but I put up a quick PR that should address your case/concerns. Tests attached that also encompasses your last snippet. When |
Thank you!
Yup, I very much agree with this 🙂 |
Released in 1.1.0~! Hopefully added enough warning labels for the general use case, but I guess time will tell 😆 Guess this means that TS could have used this now directly, although I'm sure they already have their recursive |
Thank you! |
I'm looking into using this module to implement support for
exports
in Jest.Seems unfortunate that Node's defaults should be forced on everybody using
exports
field. For a module that isn't supported to run in Node, it should just fail to resolve or something (or falling back todefault
) not pick upnode
.There's already a switch in this library for
browser
condition so it's not consistently enforced (at least from my understandingbrowser
is just a thing Node acknowledges exists, nowhere in the spec does it say its presence removes thenode
condition) - why not just fully let the caller provide their own conditions (barring possiblydefault
- that seems reasonable to always add)? I.e. remove the twoallows.add
calls (and maybe the options leaving onlyconditions
). Or possibly addnode
if noconditions
is passed, but otherwise add nothing.As a datapoint, TypeScript could not have used
resolve.exports
to implement support fortypes
condition with their official example: https://devblogs.microsoft.com/typescript/announcing-typescript-4-5-beta/#packagejson-exports-imports-and-self-referencing since this module would have resolvedimport
first.I think
resolve.exports
should be implementation/host independent (maybe aresolve.exports-node
or just aimport { node } from 'resolve.exports'
makes sense, but the base export shouldn't put these limitations there).Tl;dr: removing
resolve.exports/src/index.js
Lines 71 to 72 in c6814c4
exports
support without getting Node semantics/conditions when the module might not even run in Node or a browser.This is mostly a copy paste of yarnpkg/berry#2431 (comment).
The text was updated successfully, but these errors were encountered: