-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
module: exports patterns #34718
module: exports patterns #34718
Conversation
Review requested:
|
bd35bdd
to
ab52d12
Compare
I'm a bit concerned about two pieces here:
[1] Relevant import maps issue: WICG/import-maps#7 |
All that said: I am very excited about this refactoring! |
Note as well that if we no longer have the ability to generate an import map directly from package.json, then many of the arguments against automatic index and extension resolution no longer apply, so this will have an impact on that discussion. |
The semantics in [1] are actually incredibly similar. Thanks for pointing this out I had forgotten about that!
That will take many years at this rate. Another good reason to have the import maps discussion - Node.js does not have to be a "passive player" when it comes to import maps, it might have a role in actively driving forward concepts around them.
@ljharb the pattern can be seen as a file system based expansion to build the full import map. That is import map = function (package.json + file system). Agreed the same can be said for extension searching. Bare specifiers, even with subpaths, are a very separate case to relative though. We do not have extension searching on the agenda though, do you want to re-add a discussion / proposal here? |
@guybedford I think it can wait on the outcome of this. We've already debated extension searching extensively, and recently discussed how its proponents feel frustrated that "no searching" seems to have won via inertia. However, if the landscape changes by adding this proposal, I would be happy to add an agenda item to discuss enabling normal node resolution by default for ESM. |
@ljharb ok, that would likely set this proposal back though. Did you follow my argument wrt bare specifier extension searching versus relative specifier extension searching? |
I don't think they're actually different at all (in the absence of "exports", or in a LHS-exposed directory). In node, specifiers are specifiers, it's just that the filesystem context might determine what file is loaded. |
@ljharb the difference is that with |
@guybedford ah, i see what you're saying. I think that "relative paths below package.json" and "bare identifiers" should both be fine in that regard, and I'm still of the position that ESM should never allow filesystem-absolute paths anyways. |
ab52d12
to
6a6c27e
Compare
I've rebased this PR to a separate refactoring PR in #34744. |
6a6c27e
to
5ea9e19
Compare
@guybedford I've updated the title based on the refactor being refactored :P |
7af4a8d
to
07ba8b3
Compare
Any further feedback on this PR @nodejs/modules-active-members? I still think this is a necessary extension for the sorts of use cases we have to deal with for large numbers of exports, where directory exports may expose unwanted modules and force the usage of file extensions. The patterning system is based on always ending in a wildcard, with arbitrary number of replacements of that pattern on the RHS. If anyone has use cases, feedback or suggestions or edits to this model please say so. Any explicit concerns would be great to hear as well. |
07ba8b3
to
ac6401a
Compare
One important property of this that convinced me: It allows writing generic specifiers for dual packages. This may be worth mentioning as an example? "exports": {
"./features/*": {
"require": "./features/*.cjs",
"import": "./features/*.mjs"
}
}
// Now both of these work:
require('pkg/features/some-feature');
import('pkg/features/some-feature'); |
This PR implements an exports pattern scheme, as discussed in nodejs/modules#535. The gist of the implementation is that exports entries can be written:
such that
import 'pkg/features/x'
is written intopkg/dist/features/x/x.js
.This PR has been based to the refactoring in #34744.
One of the complexities of the implementation was that currently path exports support extension searching for CommonJS, while these new pattern exports don't. To distinguish these cases properly the exports resolution function in both the spec and implementation has been upgraded to return an object with a boolean flag indicating this state.
Previously validations of target subpaths were based on ensuring the subpath belonged to the target folder, which is no longer possible with patterns. Instead, we validate that the replacement components contain no
.
,..
ornode_modules
segments to disallow the backtracking and node_modules paths as before.Whether this should merge or not comes down to whether the usefulness justifies the extra complexity. The actual implementation complexity for this ended up not being very large with most of this PR being the refactoring (hence the negative diff!). Adding the modules agenda label, but this is already on the agenda for the meeting under the modules group issue.
Use cases gist with more examples: https://gist.github.com/guybedford/4cb418f93ef51f81343d711bbcd80dd5
@nodejs/modules-active-members
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes