| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
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). |
Sorry, something went wrong.
|
@aduh95 the test/es-module/test-esm-exports-deprecations.mjs test should definitely be failing for that? |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
You're right, I guess I tested using the wrong binary. |
Sorry, something went wrong.
There was a problem hiding this comment.
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 👍
Sorry, something went wrong.
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
PR-URL: #44477 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me>
PR-URL: #44477 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me>
|
This is amazing; sadly, it is not landing cleanly in the v16.x release branch; If you could backport this to v16.x, awesome! |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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:
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