Review requested:

• @nodejs/modules

@aduh95 nice quick work, and good test cases. We should make sure that this normalization applies in all suitable paths. Note also that any changes to the exports resolution require a corresponding spec change.

There are two options for this - normalization and validation. For normalization we would add an initial line to PACKAGE_IMPORTS_EXPORTS_RESOLVE:

**PACKAGE\_IMPORTS\_EXPORTS\_RESOLVE**(_matchKey_, _matchObj_, _packageURL_,
_isImports_, _conditions_)
> 1. Replace in _matchKey_ any _"//"_ with _"/"_.

For validation, we would update the existing validations in PACKAGE_TARGET_RESOLVE to throw a validation error for this case like we do for node_modules / percent encodings etc. We would then replace this line:

>       If _subpath_ split on _"/"_ or _"\\"_ contains any _"."_, _".."_, or
>       _"node\_modules"_ segments, case insensitive and including percent
>       encoded variants, throw an _Invalid Module Specifier_ error.

With:

>       If _subpath_ split on _"/"_ or _"\\"_ contains any empty, _"."_, _".."_, or
>       _"node\_modules"_ segments, case insensitive and including percent
>       encoded variants, throw an _Invalid Module Specifier_ error.

I'm ok with validation or normalization. Perhaps slightly tending towards preferring validation since I can't imagine a scenario where supporting these double slashings would be desired. Where normalization in general opens up more vectors, we probably want to reduce vectors.

The following code paths / test cases come to mind as needing to be included in this PR as well:

• Trailing slash exports
• Exports without a trailing slash or wildcard
• Imports without a trailing slash or wildcard
• All the above for CommonJS require as well

The following code paths / test cases come to mind as needing to be included in this PR as well:

• Trailing slash exports
• Exports without a trailing slash or wildcard
• Imports without a trailing slash or wildcard
• All the above for CommonJS require as well

IIUC, except the first point all of those test cases are already covered by the tests as is. Or am I missing something?

I didn't thoroughly check the tests, CommonJS ones are there and it does seem many are included. Here is a list of some other cases:

{
  "exports": {
    "./expt//": "./mapping/",
    "./expt//sub/": "./mapping/sub/",
    "./expt//exact": "./mapping/exact.js"
  }
}

With corresponding variations for imports.

Did you have any further feedback on validation versus normalization?

Here is a list of some other cases:

{
  "exports": {
    "./expt//": "./mapping/",
    "./expt//sub/": "./mapping/sub/",
    "./expt//exact": "./mapping/exact.js"
  }
}

With corresponding variations for imports.

Wasn't that removed in v17.0.0?

Did you have any further feedback on validation versus normalization?

IMO it's totally OK to let users define and/or use multiple slashes in a row in their exports/imports, and also it makes sense for when defining a null exports to make sure it cannot be jumped over. So normalizing all the paths wouldn't make sense IMO, only the one that are defined to null, but maybe that'd be to difficult to write in spec language.

It would be very confusing if we allowed an author to define foo/a and foo//a to be different files - it seems like validation is a much safer approach.

It would be very confusing if we allowed an author to define foo/a and foo//a to be different files - it seems like validation is a much safer approach.

I don't know, HTTP let you do that, and that doesn't seem to cause much harm.

I heavily disagree - it's caused a TON of harm, as have most of the ways the web is lax about structure.

Wasn't that removed in v17.0.0?

Just because it was deprecated doesn't mean we don't still support it and have tests for it.

I'm ok with both approaches, but for normalization we very specifically need to apply it more generally something like the suggested spec text.

I do think validation would be safer though overall, again if done carefully with respect to the spec work and edge cases.

Superseded by #44477.

@aduh95 I wonder if it's time to start thinking about the upcoming runtime deprecations?

@guybedford Runtime deprecation has landed on v19.x (#44495), so I guess the next step would be to remove it completely.

Ahh, yes then I think we have a number of runtime deprecations that we should probably remove for 20?

We could always decide to wait on some of the more recent deprecations though as well.