module: exports & imports invalid slash deprecation#44477
Merged
aduh95 merged 10 commits intonodejs:mainfrom Sep 11, 2022
Merged
module: exports & imports invalid slash deprecation#44477aduh95 merged 10 commits intonodejs:mainfrom
aduh95 merged 10 commits intonodejs:mainfrom
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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*" }forimport 'pkg/x/z.js- invalid matched pattern starting with a/{ "./*": "./x*" }forimport 'pkg/xz/- invalid matched pattern ending with a/{ "./*": "./*x" }forimport '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///xwhere 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 intopkg/xy.jsas well asimport 'pkg/x/y'resolving intopkg/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