Skip to content

module: exports & imports invalid slash deprecation#44477

Merged
aduh95 merged 10 commits intonodejs:mainfrom
guybedford:exports-double-slash-validate
Sep 11, 2022
Merged

module: exports & imports invalid slash deprecation#44477
aduh95 merged 10 commits intonodejs:mainfrom
guybedford:exports-double-slash-validate

Conversation

@guybedford
Copy link
Contributor

@guybedford guybedford commented Sep 1, 2022

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

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. esm Issues and PRs related to the ECMAScript Modules implementation. experimental Issues and PRs related to experimental features. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multiple slashes can bypass null exports path

5 participants