Skip to content
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

Closed
wants to merge 2 commits into from
Closed

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Aug 11, 2020

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:

{
  "exports": {
    "./features/*": "./dist/features/*/*.js"
  }
}

such that import 'pkg/features/x' is written into pkg/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 ., .. or node_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), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. esm Issues and PRs related to the ECMAScript Modules implementation. labels Aug 11, 2020
@jkrems
Copy link
Contributor

jkrems commented Aug 11, 2020

I'm a bit concerned about two pieces here:

  1. The "*" syntax was previously valid as an exports pattern afaik and also may end up appearing in import maps, potentially with different semantics[1]. So we're potentially conflicting with two things here.
  2. If this or something very similar doesn't end up in import maps, this is the moment where just the package.json data is no longer enough to convert exports to an import map. Building an import map now requires downloading the package contents and scanning for matches. Ad-hoc resolution still works fully in-memory (no file access required) but import map-based resolution[2] doesn't.

[1] Relevant import maps issue: WICG/import-maps#7
[2] By "fully in-memory import map-based resolution" I mean getting the package.json metadata, transforming it into an import map, and then running "vanilla" import map resolution based on it - all without having access to the package contents.

@jkrems
Copy link
Contributor

jkrems commented Aug 11, 2020

All that said: I am very excited about this refactoring!

@ljharb
Copy link
Member

ljharb commented Aug 11, 2020

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
Copy link
Contributor Author

The "*" syntax was previously valid as an exports pattern afaik and also may end up appearing in import maps, potentially with different semantics[1]. So we're potentially conflicting with two things here.

The semantics in [1] are actually incredibly similar. Thanks for pointing this out I had forgotten about that!

If this or something very similar doesn't end up in import maps

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.

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.

@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
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor Author

@ljharb the difference is that with "exports" patterns there exists a processing step to enumerate all "exports" from the pattern by looking at the files in the package, because the space of all possibilities of specifiers within the package is full enumerated / encapsulated by exports. With relative or absolute specifiers, the space of all possibilities is the entire user file system - not something which is practical to enumerate.

@ljharb
Copy link
Member

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.

@guybedford
Copy link
Contributor Author

guybedford commented Aug 12, 2020

I've rebased this PR to a separate refactoring PR in #34744.

@MylesBorins MylesBorins changed the title module: exports patterns and resolver refactoring module: exports patterns Aug 12, 2020
@MylesBorins
Copy link
Contributor

@guybedford I've updated the title based on the refactor being refactored :P

@guybedford guybedford force-pushed the exports-patterns branch 3 times, most recently from 7af4a8d to 07ba8b3 Compare August 13, 2020 19:01
@guybedford guybedford added the modules-agenda Issues and PRs to discuss during the meetings of the Modules team. label Sep 2, 2020
@guybedford
Copy link
Contributor Author

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.

doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
@jkrems
Copy link
Contributor

jkrems commented Sep 2, 2020

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');

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. esm Issues and PRs related to the ECMAScript Modules implementation. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.