Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.2k
module: exports patterns#34718
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
guybedford commented Aug 11, 2020 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
nodejs-github-bot commented Aug 11, 2020
Review requested:
|
bd35bdd to ab52d12Comparehybrist commented Aug 11, 2020
I'm a bit concerned about two pieces here:
[1] Relevant import maps issue: WICG/import-maps#7 |
hybrist commented Aug 11, 2020
All that said: I am very excited about this refactoring! |
ljharb commented Aug 11, 2020 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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. |
guybedford commented Aug 11, 2020
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? |
ljharb commented Aug 11, 2020
@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. |
guybedford commented Aug 11, 2020
@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? |
ljharb commented Aug 11, 2020
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. |
guybedford commented Aug 11, 2020
@ljharb the difference is that with |
ljharb commented Aug 11, 2020
@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 6a6c27eCompareguybedford commented Aug 12, 2020 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
I've rebased this PR to a separate refactoring PR in #34744. |
6a6c27e to 5ea9e19CompareMylesBorins commented Aug 12, 2020
@guybedford I've updated the title based on the refactor being refactored :P |
7af4a8d to 07ba8b3Compareguybedford commented Sep 2, 2020
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 ac6401aCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
hybrist commented Sep 2, 2020 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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'); |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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_modulessegments 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