-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 support for package exports into .pnp.js #1359
Conversation
I think we should refrain from doing that - third-party resolvers that already make use of Overall I'm a bit worried of the size of the implementation - it's a lot of code just to workaround the fact that we cannot directly call the equivalent of I've also opened nodejs/node#33423 which, if fixed, would simplify that a bit (we would still need a way to access the resolver at mid-level, since otherwise we would send absolute paths to the resolver which would then ignore the |
It's not just the 200+ lines of code that worry me. The fact that we implement the algorithm ourselves is potentially an issue. Right now it would work fine, but it breaks down if changes to this algorithm are made in the future. For example, something they've left open in the algorithm: re-exporting packages. Here's an example of how a package could use that to conditionally load a CSS preprocessor:
We would need to change the behaviour in Moving this to a node API would benefit not just us but—I hope—the entire ecosystem of bundlers and loaders. Right now node has documented the algorithm but hardcoded it into the cjs and esm loaders. If e.g. typescript wants to support Without support in node itself, typescript is stuck between a rock and a hard place:
I'm using typescript as example here, but the same goes for any project that reads a file path out of the package manifest. Browserify, webpack, rollup etc. might want to pass in extra / different options, e.g. The Angular CLI has just moved from a custom implementation of the node_modules algorithm to using // node_modules/with-exports/package.json
{
"name": "with-exports",
"exports": "./index.js"
} // test.js
console.log(require.resolve('with-exports/package.json'));
|
Yep, I totally agree with that. Imo the best would even be for the Node project to publish an official @guybedford I'm curious what you would think about that, since you worked quite a bit on the Node resolution pipeline if I remember correctly? Do you think there could be any chance for something like this to happen? |
@arcanis I agree the ideal would be an official package. If someone is interested in working on this I'm sure it could get momentum - the main thing is that a userland package needs hooks into resolver features like extensions etc, so working out those details needs to be done. Personally my hope was that by putting together a very clear specification for the resolver, it would be easy for other tools to build on top of it since the semantics are well-defined. For example in jspm I borrow semantics as necessary but don't need an actual resolver implementation since the package lookup process is so different. I am surprised no one has put together a very good userland resolver implementation yet and would encourage people to work on either this problem or trying to get that work directly in core too. I do think userland may be a better fit to ensure speed of changes that meet all the configurations and userland needs, but if you feel core would be better I have nothing against that too. |
Note the specification here describes the edge cases and errors in detail, and has been carefully maintained to match the implementation - https://nodejs.org/dist/latest-v14.x/docs/api/esm.html#esm_resolver_algorithm (you have to expand the "Resolver algorithm specification" section). |
We've discussed with @bgotink and came to the conclusion that in the current state of things, maybe the best would be to contribute the This way we could remove the qualified path resolution from our codebase, focusing exclusively on bare specifiers. |
This sounds like a great collaboration, thank you @arcanis for taking the time to dig deeper here and find a solution that can help many other projects too! If there are any questions or issues integrating the resolution changes, I'm sure @ljharb will be able to advise on this, and feel free to ping me as well. And if there are any difficulties or feedback you have on the implementation or spec do feel free to post an issue on the modules repo too. |
Happy to help any way I can. |
This is just a first iteration, and even then the code is far from finished.
What's the problem this PR addresses?
Berry doesn't support
exports
in package.json#650, #1277
How did you fix it?
This PR currently contains a very basic approach which loads the exports at resolution time, validates the exports and then applies the exports onto the unqualified path to get a new unqualified path. The algorithm has been implemented as per node documentation (https://nodejs.org/api/modules.html#modules_all_together, https://nodejs.org/api/esm.html#esm_resolver_algorithm).
As I noted in #650 (comment), package exports resolution currently fits neither in
resolveToUnqualified
orresolveUnqualified
, so I've placed it into a separate function.Alternatively we could validate the exports at installation time and store it in PnP data. That will result in "invalid metadata" errors at installation time instead of at resolution time (fail fast). It also means one less file system hit per bare import resolution. Finally, it'll make it possible for
resolveToUnqualified
to handle export mapping.I'm not 100% happy with the current approach, if only because
resolveToUnqualified
returns an invalid path for packages that have exports. It feels weird to then go map that onto a valid path.