Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

module: deprecate trailing slash pattern mappings #40039

Closed
wants to merge 5 commits into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Sep 8, 2021

This PR deprecates the ability for pattern mappings to be able to resolve import specifiers ending in / like import('pkg/subpath/'), with a new deprecation warning and message.

The primary motivation here is that the import maps specification does not support trailing / mappings resolving.

For example:

<!doctype html>
<script type="importmap">
{
  "imports": {
    "pkg/subpath/": "/pkg/index.js"
  }
}
</script>
<script type="module">
import 'pkg/subpath/';
</script>

will give the browser console error:

Ignored an import map value of "pkg/subpath/": Since specifierKey ended in a slash, so must the address: /pkg/index.js

See WICG/import-maps#244 for more background here.

Since we deprecated folder subpaths, there are no common patterns of trailing / usage yet. It is only with the introduction of pattern trailers in #39635 that it is now easy to define maps like:

{
  "exports": {
    "./*/": "./*/index.js"
  }
}

to support import('pkg/asdf/') resolving to pkg/asdf/index.js for any name.

The risk is that users start defining packages like this and then these packages will never be supportable in import maps environments. By deprecating this now we can avoid this outcome.

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Sep 8, 2021
@guybedford
Copy link
Contributor Author

@nodejs/modules

doc/api/deprecations.md Show resolved Hide resolved
doc/api/deprecations.md Outdated Show resolved Hide resolved
@aduh95 aduh95 added deprecations Issues and PRs related to deprecations. semver-major PRs that contain breaking changes and should be released in the next major version. labels Sep 8, 2021
@guybedford guybedford added deprecations Issues and PRs related to deprecations. and removed semver-major PRs that contain breaking changes and should be released in the next major version. deprecations Issues and PRs related to deprecations. labels Sep 8, 2021
doc/api/deprecations.md Outdated Show resolved Hide resolved
doc/api/deprecations.md Outdated Show resolved Hide resolved
Co-authored-by: Antoine du Hamel <[email protected]>
@ljharb
Copy link
Member

ljharb commented Sep 8, 2021

What about core modules? It's a very common pattern to require, eg fs/ to indicate that the user wants node_modules/fs and not the core fs module.

… or does this only apply to exports?

What about wanting to disambiguate foo.js from foo/index.js, and wanting the user to do $pkg/foo vs $pkg/foo/?

@guybedford
Copy link
Contributor Author

@ljharb now that we have node:fs I think that's the better pattern for distinction going forward.

And yes, disambiguation of pkg/foo versus pkg/foo/ in the import maps specification not being possible is exactly the issue here.

test/es-module/test-esm-exports-deprecations.mjs Outdated Show resolved Hide resolved
Co-authored-by: Antoine du Hamel <[email protected]>
@ljharb
Copy link
Member

ljharb commented Sep 8, 2021

@guybedford for a core module, yes, i can use node: as a prefix, but i'd still need fs/ to be able to get at node_modules/fs - the prefix doesn't change that.

And yes, disambiguation of pkg/foo versus pkg/foo/ in the import maps specification not being possible is exactly the issue here.

Why are we reducing the usefulness of node because an entirely different specification - one node doesn't even follow - lacks functionality?

@aduh95
Copy link
Contributor

aduh95 commented Sep 8, 2021

To be clear, you mean being able to do something like that, correct?

{
  "imports": {
    "#fs": "fs/"
  }
}

And import '#fs' would load node_modules/fs package. But I don't think this PR forbids that, does it?

@guybedford
Copy link
Contributor Author

guybedford commented Sep 9, 2021

Note that import 'fs/' resolving node_modules/fs/index.js is deprecated due to folder mappings.

So this is just fs/subpath/ use cases.

Why are we reducing the usefulness of node because an entirely different specification - one node doesn't even follow - lacks functionality?

So this PR only reduces the functionality of a feature landed two days ago with pattern trailers.

@nodejs-github-bot
Copy link
Collaborator

@ljharb
Copy link
Member

ljharb commented Sep 9, 2021

By deprecated, you mean in ESM, or in CJS?

Either way, if I have a folder in node_modules/fs with a package.json with an "exports" with a dot in it, how do you propose i both require and import it?

In CJS, require('fs/') would be the answer; you seem to be suggesting that in ESM there's no non-deprecated answer. That seems like a problem.

@guybedford
Copy link
Contributor Author

By deprecated, you mean in ESM, or in CJS?

We deprecated it specifically for ESM per the folder mappings deprecation PR.

Yes, ESM does deprecate the ability to load npm packages with the same names as core modules, but this is completely off topic at this point for this PR - I'd suggest opening another issue to discuss that.

guybedford added a commit that referenced this pull request Sep 15, 2021
PR-URL: #40039
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
@guybedford
Copy link
Contributor Author

Landed in 7216fb1.

@guybedford guybedford closed this Sep 15, 2021
@guybedford guybedford deleted the restrict-exports-slash branch September 15, 2021 06:59
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
PR-URL: #40039
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
PR-URL: #40039
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Sep 21, 2021
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecations Issues and PRs related to deprecations. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants