modules icon indicating copy to clipboard operation
modules copied to clipboard

Empty path matching in patterns

Open guybedford opened this issue 5 years ago • 8 comments

@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.

guybedford avatar Sep 18 '20 22:09 guybedford

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.

hybrist avatar Sep 18 '20 22:09 hybrist

@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!

guybedford avatar Sep 18 '20 22:09 guybedford

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

ljharb avatar Sep 18 '20 23:09 ljharb

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 avatar Sep 18 '20 23:09 guybedford

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

ljharb avatar Sep 19 '20 07:09 ljharb

@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 //.

guybedford avatar Sep 21 '20 01:09 guybedford

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.

ljharb avatar Sep 21 '20 04:09 ljharb

@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.

guybedford avatar Sep 21 '20 08:09 guybedford