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

Empty path matching in patterns #49447

Open
guybedford opened this issue Sep 18, 2020 · 8 comments
Open

Empty path matching in patterns #49447

guybedford opened this issue Sep 18, 2020 · 8 comments
Labels
module Issues and PRs related to the module subsystem.

Comments

@guybedford
Copy link
Contributor

guybedford commented Sep 18, 2020

@ljharb brought up in the exports patterns PR that it would be worthwhile to ensure the * matching in exports patterns permits empty matches to ensure consistency with all other wildcard schemes.

I agree this conceptual integrity could make sense, and in addition such a feature could be added as a fully backwards compatible change to exports patterns so I would be glad to see this land either way.

One benefit I really liked of not matching the null match is that currently:

{
  "exports": {
    "./": "./sub/"
  }
}

will cause import.meta.resolve('pkg/') to resolve into the sub/ folder. As a result this means import.meta.resolve('pkg/') is not a reliable way to get a package root (and also relates to the package.json resolution discussion previously).

If we now use patterns here:

{
  "exports": {
    "./*": "./sub/*"
  }
}

then as currently written, import.meta.resolve('pkg/') will throw a PACKAGE_PATH_NOT_EXPORTED error because the empty string match is not permitted and thus this "reserves" this space for us to treat it as a special package base resolution case which seems a really useful property for a resolver to have to me in being able to resolve the base of any package this way.

On the other hand I do not have any conceptual arguments against matching the empty string pattern other than that I simply can't think of a single use case in which it is useful. If the argument is purely one of conceptual consistency being important I'm all for that though, but would be concerned about invalidating a perfectly good use case in the name of that.

@jkrems
Copy link
Contributor

jkrems commented Sep 18, 2020

I will throw in that if we make ./sub/* not match pkg/sub/, then we should also make ./sub/ not match pkg/sub/ (assuming that's still possible which I think it is). If we want to encourage people to always use patterns even when prefix would technically also work, it feels weird if there's this one case where an exports can be expressed using prefix but not using pattern.

@guybedford
Copy link
Contributor Author

@jkrems we were discussing a multi-major deprecation of path exports. At the current rate of progress on anyone having a clue how to build a standard around import.meta.resolve this is the time frame we are looking for that landing too!

@ljharb
Copy link
Member

ljharb commented Sep 18, 2020

Remember as well that these paths are also used for CJS, and require('pkg/') with CJS's automatic resolution is supremely useful.

@ljharb ljharb mentioned this issue Sep 18, 2020
4 tasks
@guybedford
Copy link
Contributor Author

Ah that's an interesting case. Does that work currently with exports resolution to resolve eg index? If so, I'm not sure the group as a whole made a decision to support that explicitly.

@guybedford guybedford added the modules-agenda Issues and PRs to discuss during the meetings of the Modules team. label Sep 18, 2020
@ljharb
Copy link
Member

ljharb commented Sep 19, 2020

Yes, it does (for CJS), and yes, we definitely did. Only ESM has limited resolution inside a / export.

@guybedford
Copy link
Contributor Author

@ljharb I wrote the PR and I didn't know about this case! I disagree in thinking the answer to the fact that require('pkg/') with an "exports": { "./": "./" } entry will resolve pkg/index.js was formerly understood all members of this group let alone explicitly endorsed.

Here is an example of a case where empty string matches might unintentially expose modules:

{
  "exports": {
    "./*": "./*/main.js
  }
}

Where there exist subfolders like "feature1", "feature2" etc to support import 'pkg/feature1'.

I do not think it would be obvious that import 'pkg/' would resolve to pkg/main.js in this case. This is because it resolves to .//main.js which resolves in the URL resolver to dedupe the double //.

@ljharb
Copy link
Member

ljharb commented Sep 21, 2020

You can run npx ls-exports in a package if you'd like to see how many CJS entrypoints are exposed in post-ESM node, for a given package/dir. Clearly everyone wasn't aware of it, but we explicitly discussed it in the modules meeting on more than one occasion, to my recollection.

@guybedford
Copy link
Contributor Author

@ljharb what is exposed and what is actually used are two different things though. We still have an opportunity to fix minor issues like this that may have slipped through.

@GeoffreyBooth GeoffreyBooth transferred this issue from nodejs/modules Sep 1, 2023
@GeoffreyBooth GeoffreyBooth added module Issues and PRs related to the module subsystem. and removed modules-agenda Issues and PRs to discuss during the meetings of the Modules team. labels Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants