module: exports & imports invalid slash deprecation #44477
Conversation
|
Review requested:
|
nodejs-github-bot
added
esm
guybedford
changed the title
|
Thanks for doing this!
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. |
|
@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. |
|
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 |
|
@aduh95 the |
|
@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. |
|
You're right, I guess I tested using the wrong binary. |
Co-authored-by: Antoine du Hamel <[email protected]>
aduh95
added
experimental
|
Landed in 53633c0 |
PR-URL: nodejs#44477 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]>
PR-URL: #44477 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]>
PR-URL: #44477 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]>
PR-URL: #44477 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]>
|
This is amazing; sadly, it is not landing cleanly in the v16.x release branch; If you could backport this to v16.x, awesome! |
|
@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. |
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