module: exports & imports invalid slash deprecation

[guybedford]

Fixes #44316 with an alternative to #44328 based on validation instead of normalization.

This PR creates a documentation-only deprecation for double slashes (//) in exports targets, subpaths and patterns available via --pending-deprecations. It also deprecates pattern matches starting with or ending with / as these could also create double slashing when substituted into a position immediatel before or after a slash.

The expectation for this PR would be to move to a runtime deprecation for the next major release, before moving to a validation error in a subsequent release.

Examples of deprecated cases for values of the "exports" field that would give an invalid target error when this moves to a runtime validation:

• { "./x": ".//x/.js" } - invalid double slash in target
• { "./*": "./x*" } for import 'pkg/x/z.js - invalid matched pattern starting with a /
• { "./*": "./x*" } for import 'pkg/xz/ - invalid matched pattern ending with a /
• { "./*": "./*x" } for import 'pkg//x' - invalid matched pattern starting with a /

Once the deprecation has been fully made, this would then effectively fix the case in #44316 by throwing a validation error.

We still allow cases such as import 'pkg///x where there is an exports entry { ".///x": "./x.js" }, that is - the validation applies to the target and target replacements and not to the left hand match.

The disabling of leading and trailing / in pattern matching may be seen to be the most risky here, since there might be valid usage of:

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

supporting import 'pkg/xy' resolving into pkg/xy.js as well as import 'pkg/x/y' resolving into pkg/x/y.js.

If there is pushback on such cases, we could further restrict the pattern matching aspect to only disable cases that really end up resolving into double slashes, but I couldn't think of a case where the above matching / boundaries really would be wanted practically so it seems better off to make the restriction.

For review, I would advise starting with the spec diff (which also includes some unrelated refactorings), then the tests. Some minor refactorings are also included in the code.

//cc @nodejs/modules

[nodejs-github-bot]

Review requested:

• @nodejs/modules

[aduh95]

Thanks for doing this!

Fixes #44316

Can we add the test cases that test for that use case? You can copy them from my PR, or I can do it if it's OK for me to push to your branch.

[guybedford]

@aduh95 thanks for being open to the approach. Feel free to push commits - if you have time to bring the cases through that would be a huge help.

[aduh95]

Hum so I did pull the tests from the other PR, it looks like the deprecation doesn't affect the linked issue, there are still ways to escape a null defined subpath (e.g. import 'pkgexports/sub/////internal/////test.js' does not trigger any pending deprecation warning).

[guybedford]

@aduh95 the test/es-module/test-esm-exports-deprecations.mjs test should definitely be failing for that?

[guybedford]

@aduh95 I've just pushed up those deprecation tests, which demonstrates it's catching those. Let me know if you can see anything missing though.

[aduh95]

You're right, I guess I tested using the wrong binary.

[JakobJingleheimer]

LGTM! Thanks!

I think throwing in this instance is better than trying to do some hocus pocus behind the scenes to protect the user from themselves 👍

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/46437/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/46502/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/46507/

[aduh95]

Landed in 53633c0

[juanarbol]

This is amazing; sadly, it is not landing cleanly in the v16.x release branch; If you could backport this to v16.x, awesome!

[guybedford]

@juanarbol thanks for the update, a backport to 16 requires working through the interaction with the trailing slash export deprectation which isn't necessarily trivial. I think we may be ok to just leave this on 18 actually, since it's part of the 18 -> 20 upgrade path at this point.