Review requested:

• @nodejs/modules

Codecov Report

Merging #35747 into master will decrease coverage by 8.49%.
The diff coverage is 98.03%.

@@            Coverage Diff             @@
##           master   #35747      +/-   ##
==========================================
- Coverage   96.41%   87.91%   -8.50%     
==========================================
  Files         223      477     +254     
  Lines       73685   113133   +39448     
  Branches        0    24642   +24642     
==========================================
+ Hits        71043    99462   +28419     
- Misses       2642     7963    +5321     
- Partials        0     5708    +5708     

@addaleax This is a runtime deprecation on two experimental features (subpath exports and subpath imports), so maybe it's not semver-major?

@Trott Yeah, no strong opinion, it felt like it’s better to err on the safe side here to me, that’s all :)

Thinking some more about the backporting I wonder if it might actually be useful to backport this to 14.

The warnings will only show for local package usage since we have good filtering to not show for third-party packages without -pending-deprecation.

I would still suggest holding out until 16/17 for any throw-by-default behaviour though, but it could be beneficial to backport the warning actually.

I'm updating the PR description, but if anyone has any concerns let me know.

For all the modules experimental status process and changes, this is probably one of the only few removals we've done actually!

@addaleax given the above, would you be ok with removing the semver major label here?

@guybedford As I said, no strong opinion :) Just the fact that runtime deprecations are semver-major by default, but if everybody else is good with it then I am too

Just the fact that runtime deprecations are semver-major by default, but if everybody else is good with it then I am too

Ah I was not aware of that. Ok I'm completely fine with leaving the backport then.

I think doing this as non-semver major but not backporting is a nice "split the difference"

So what about the "./": "./" mapping that we’ve been advising people to add to avoid making adding "exports" a breaking change? That needs to become "./*": "./*" or something?

I don’t believe those are equivalent because package/ wouldn’t work with the pattern.

Certainly there is that tailing "/" difference between the two which is deprecated here.

For the use cases in the readme covered by "./": "./" we already suggest using "./*": "./*". Further docs suggestions very welcome too.

(To be clear tho, removing the slash feature makes it impossible to make adding "exports" a non-breaking change - unless you could add "./": "./index.js" and that would work?)

That's true, although we already advise that exports typically is a breaking change. The import 'pkg/' edge case is likely at the bottom of the list of cases users have to consider and is a very rare pattern in the wild, typically only used for package names that match Node.js builtins to get around builtin detections so I don't think it realistically affects most packages.

With this trailing separator deprecation, in theory it could allow a future implementation space to support these as individual maps in their own right.

CI: https://ci.nodejs.org/job/node-test-pull-request/33806/