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: support pattern trailers #39635

Closed
wants to merge 5 commits into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Aug 2, 2021

This implements an extension to exports (and imports) patterns to support LHS matches consisting of trailing strings.

The use case for this came out of discussion with @ljharb, in order to support packages that want to provide "exports" support while enabling subpaths that can support both extensioned and unextensioned forms.

For example, with this PR, the following exports field can be used to expose a features folder with optional extensions:

{
  "exports": {
    "./features/*": "./features/*.js",
    "./features/*.js": "./features/*.js"
  }
}

While we would still encourage package authors to decide on a single pattern, often packages don't have a choice when backwards compatibility is required, so this can also allow for smoother interface transitions if deprecating one or the other pattern as well.

This was left out of the initial implementation to keep the initial implementation as simple as possible. Most of the work is just clearly defining pattern specificity and handling edge cases.

This PR also refines the following edge cases:

  1. Trailing "/" exports that resolve to files will now give the trailing "/" deprecation warning.
  2. Given the two exports keys "./x/" and "./x*", the second pattern will take precedence over the path for resolving ./x/y according to the specificity rules.
  3. Multiple * can't be matched in patterns, so given "exports": { "./*/*.js": "./*.js" } this will no longer match import './*/x.js' and instead throw an unexported error. It is of course possible to resolve a * as part of the user specifier component, it just won't be supported as a string character in exports.

@guybedford
Copy link
Contributor Author

@nodejs/modules

@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 Aug 2, 2021
@ljharb
Copy link
Member

ljharb commented Aug 2, 2021

What will happen to a package using pattern trailers, in a version of node that supports subpath patterns but not pattern trailers?

@guybedford
Copy link
Contributor Author

@ljharb it would simply be ignored as was reserved for the forwards compatibility path here. In the above example, ./features/* would be matched.

@ljharb
Copy link
Member

ljharb commented Aug 2, 2021

Great! It’s a shame this wasn’t in the initial version, but it’s a great improvement now.

lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Aug 3, 2021

Before this lands, is there any remotely easy way i can test it locally against my package being imported by a dependency?

@guybedford
Copy link
Contributor Author

@ljharb the best way is always to take the time to clone and run the build locally. Perhaps @MylesBorins or @targos would be able to trigger a special build but I'm not sure of the process there.

@targos
Copy link
Member

targos commented Aug 3, 2021

Test build: https://ci-release.nodejs.org/job/iojs+release/7075/

I'll update this message when it's available to download.

@guybedford
Copy link
Contributor Author

Turns out there was an issue with the feedback changes, just pushed up a fix.

@targos thanks for triggering this, is it possible to restart it?

@targos
Copy link
Member

targos commented Aug 3, 2021

@targos
Copy link
Member

targos commented Aug 3, 2021

Download links: https://nodejs.org/download/test/v17.0.0-test24288e0c26/

@guybedford
Copy link
Contributor Author

Ping for further reviewers here //cc @nodejs/modules.

@ljharb
Copy link
Member

ljharb commented Aug 6, 2021

@targos unfortunately i can't easily install that with nvm because it's not present in the index.tab :-p thanks tho, i'll try to get it installed locally.

Copy link
Member

@bmeck bmeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, not performing the actual sort seems fine as well since the list of keys is likely not huge, but it could be beneficial if we think we will see large mappings.

@ljharb
Copy link
Member

ljharb commented Aug 6, 2021

@guybedford it'd be great if i could test this before you land it; or at least before it's backported.

@MylesBorins
Copy link
Member

I'll try and find time to review this over the next week. @ljharb if we make a test build you can install via nvm, I'll create a test build and post the one liner to install via nvm.

@guybedford How far were you thinking we could backport this? I'm guessing 14 is as far back as we could go.

@ljharb
Copy link
Member

ljharb commented Aug 6, 2021

@MylesBorins the test build exists - #39635 (comment) - but it's not listed in https://nodejs.org/download/test/index.tab, so i can't in fact install it via nvm (happy to be wrong here tho)

@MylesBorins
Copy link
Member

@ljharb I just checked the CI job for the build and there was a failure that resulted in the tabfile not getting updated. Just reran in and finger crossed it should work this time.

@ljharb
Copy link
Member

ljharb commented Aug 6, 2021

@MylesBorins It's been updated (the timestamp on https://nodejs.org/download/test/ updated) but it still only contains v17.0.0-test20210702a107e9054a, and not v17.0.0-test24288e0c26 :-/

@guybedford
Copy link
Contributor Author

How far were you thinking we could backport this? I'm guessing 14 is as far back as we could go.

@MylesBorins as far back as we can go yes, 14 could be fine though.

@ljharb
Copy link
Member

ljharb commented Aug 8, 2021

I got it downloaded and installed by locally hardcoding nvm to accept v17.0.0-test24288e0c26.

Tested it with es-abstract imported by another module, and I still couldn't get all of v17.0.0-test24288e0c26, v16.6.1, v12.17, and v12.22 to be able to import the module with, or without, the extension.

The best results were without the extension - with an exports like:

"./2020/*": "./2020/*.js",
"./2020/*.js": "./2020/*.js",
"./2020/": "./2020/",

everything worked but v12.17, which had an error message about using the extension (which, when i use it, works in 17 and 12.17 but fails in latest 12 and 16).

I won't be able to use the feature if I can't maintain back compat; is there any iteration we might want to do before landing this?

@guybedford
Copy link
Contributor Author

guybedford commented Aug 9, 2021

@ljharb thanks for testing it out. 12.17 doesn't support patterns or extensions searching for path exports I believe which would explain that case. I assume by "17" in your last reference you mean this PR, which yes is the only way to support the extensioned form mapping so your results sound as expected to me. As discussed in the issue on your repo, I don't think this feature can apply to your use case at this point as, as you have seen, there is no way to support both the extensioned and unextensioned import forms while supporting all versions of Node.js and using the exports field. I can't think of a variation of this feature that could get around the compatibility.

If we had shipped this feature from day one then yes it would have solved your use case though.

@ljharb
Copy link
Member

ljharb commented Aug 9, 2021

I understand that I’m unable to support both forms; I’m asking if there’s a way to support one of them in all those node versions, even if it’s extensioned for ESM (as long as CJS is always extensionless). iow, i don’t mind if to support 12.17 means ESM must use extensions, as long as a) CJS can continue to omit them, and b) a bonus is if new code that doesn’t support 12.17 et al can use extensionless ESM.

@guybedford
Copy link
Contributor Author

The best option is yes to use the extensioned form, and then use the exports field:

"./2020/*": "./2020/*",
"./2020/": "./2020/",

That should work in all versions of Node.

Because exports applies equally to ESM and CJS, there's no way to support extensioned or not for one or the other short of using conditions to just disable certain paths.

If you want to support unextensioned, then you would use the rule:

"./2020/*": "./2020/*.js",
"./2020/": "./2020/",

This PR doesn't apply to either case, as the use case for this PR is being able to support both.

The other benefit of the trailing *.js mapping is just that it specifies the exact file extensions so "./2020/*": "./2020/*" being replaced with "./2020/*.js": "./2020/*.js" for modern Node.js support ensures only .js files are exposed publicly, allowing files with other file extensions to maintain encapsulation despite using extensioned subpaths.

@nodejs-github-bot
Copy link
Collaborator

@targos targos mentioned this pull request Sep 6, 2021
targos added a commit that referenced this pull request Sep 6, 2021
Notable changes:

crypto:
  * (SEMVER-MINOR) add RSA-PSS params to asymmetricKeyDetails (Tobias Nießen) #39851
deps:
  * (SEMVER-MINOR) add corepack (Maël Nison) #39608
  * (SEMVER-MINOR) update V8 to 9.3.345.16 (Michaël Zasso) #39947
module:
  * (SEMVER-MINOR) support pattern trailers (Guy Bedford) #39635
stream:
  * (SEMVER-MINOR) add stream.compose (Robert Nagy) #39029

PR-URL: #40011
targos added a commit that referenced this pull request Sep 7, 2021
Notable changes:

crypto:
  * (SEMVER-MINOR) add RSA-PSS params to asymmetricKeyDetails (Tobias Nießen) #39851
deps:
  * (SEMVER-MINOR) add corepack (Maël Nison) #39608
  * (SEMVER-MINOR) update V8 to 9.3.345.16 (Michaël Zasso) #39947
module:
  * (SEMVER-MINOR) support pattern trailers (Guy Bedford) #39635
stream:
  * (SEMVER-MINOR) add stream.compose (Robert Nagy) #39029

PR-URL: #40011
guybedford added a commit to guybedford/node that referenced this pull request Sep 15, 2021
PR-URL: nodejs#39635
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
guybedford added a commit that referenced this pull request Oct 8, 2021
PR-URL: #39635
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
danielleadams pushed a commit that referenced this pull request Oct 12, 2021
PR-URL: #39635
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
danielleadams pushed a commit that referenced this pull request Oct 12, 2021
PR-URL: #39635
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
guybedford added a commit to guybedford/node that referenced this pull request Oct 18, 2021
PR-URL: nodejs#39635
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
richardlau pushed a commit that referenced this pull request Nov 30, 2021
PR-URL: #39635
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
richardlau added a commit that referenced this pull request Jan 25, 2022
Notable changes:

Corepack:
Node.js now includes Corepack, a script that acts as a bridge between
Node.js projects and the package managers they are intended to be used
with during development.
In practical terms, Corepack will let you use Yarn and pnpm without
having to install them - just like what currently happens with npm,
which is shipped in Node.js by default.

Contributed by Maël Nison - #39608

ICU updated:
ICU has been updated to 70.1. This updates timezone database to 2021a3,
including bringing forward the start for DST for Jordan from March to
February.

Contributed by Michaël Zasso - #40658

New option to disable loading of native addons:
A new command line option `--no-addons` has been added to disallow
loading of native addons.

Contributed by Dominic Elm - #39977

Updated Root Certificates:
Root certificates have been updated to those from Mozilla's Network
Security Services 3.71.

Contributed by Richard Lau - #40280

Other Notable Changes:

crypto:
  * (SEMVER-MINOR) make FIPS related options always available (Vít Ondruch) #36341
lib:
  * (SEMVER-MINOR) add unsubscribe method to non-active DC channels (simon-id) #40433
  * (SEMVER-MINOR) add return value for DC channel.unsubscribe (simon-id) #40433
module:
  * (SEMVER-MINOR) support pattern trailers (Guy Bedford) #39635
src:
  * (SEMVER-MINOR) make napi_create_reference accept symbol (JckXia) #39926

PR-URL: #41696
@richardlau richardlau mentioned this pull request Jan 25, 2022
richardlau added a commit that referenced this pull request Feb 1, 2022
Notable changes:

Corepack:
Node.js now includes Corepack, a script that acts as a bridge between
Node.js projects and the package managers they are intended to be used
with during development.
In practical terms, Corepack will let you use Yarn and pnpm without
having to install them - just like what currently happens with npm,
which is shipped in Node.js by default.

Contributed by Maël Nison - #39608

ICU updated:
ICU has been updated to 70.1. This updates timezone database to 2021a3,
including bringing forward the start for DST for Jordan from March to
February.

Contributed by Michaël Zasso - #40658

New option to disable loading of native addons:
A new command line option `--no-addons` has been added to disallow
loading of native addons.

Contributed by Dominic Elm - #39977

Updated Root Certificates:
Root certificates have been updated to those from Mozilla's Network
Security Services 3.71.

Contributed by Richard Lau - #40280

Other Notable Changes:

crypto:
  * (SEMVER-MINOR) make FIPS related options always available (Vít Ondruch) #36341
lib:
  * (SEMVER-MINOR) add unsubscribe method to non-active DC channels (simon-id) #40433
  * (SEMVER-MINOR) add return value for DC channel.unsubscribe (simon-id) #40433
module:
  * (SEMVER-MINOR) support pattern trailers (Guy Bedford) #39635
src:
  * (SEMVER-MINOR) make napi_create_reference accept symbol (JckXia) #39926

PR-URL: #41696
mwalbeck pushed a commit to mwalbeck/docker-jellyfin-livestream that referenced this pull request Mar 16, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [node](https://github.com/nodejs/node) | stage | minor | `14.18.3-bullseye-slim` -> `14.19.0-bullseye-slim` |

---

### Release Notes

<details>
<summary>nodejs/node</summary>

### [`v14.19.0`](https://github.com/nodejs/node/releases/v14.19.0)

[Compare Source](nodejs/node@v14.18.3...v14.19.0)

##### Notable Changes

##### Corepack

Node.js now includes Corepack, a script that acts as a bridge between Node.js projects and the package managers they are intended to be used with during development.
In practical terms, **Corepack will let you use Yarn and pnpm without having to install them** - just like what currently happens with npm, which is shipped in Node.js by default.
Please head over to the [Corepack documentation page](https://nodejs.org/dist/latest-v14.x/docs/api/corepack.html) for more information on how to use it.

Contributed by Maël Nison - [#&#8203;39608](nodejs/node#39608)

##### ICU updated

ICU has been updated to 70.1. This updates timezone database to 2021a3, including bringing forward the start for DST for Jordan from March to February.

Contributed by Michaël Zasso - [#&#8203;40658](nodejs/node#40658)

##### New option to disable loading of native addons

A new command line option `--no-addons` has been added to disallow loading of native addons.

Contributed by Dominic Elm - [#&#8203;39977](nodejs/node#39977)

##### Updated Root Certificates

Root certificates have been updated to those from Mozilla's Network Security Services 3.71.

Contributed by Richard Lau - [#&#8203;40280](nodejs/node#40280)

##### Other Notable Changes

-   \[[`0d448eaab5`](nodejs/node@0d448eaab5)] - **(SEMVER-MINOR)** **crypto**: make FIPS related options always available (Vít Ondruch) [#&#8203;36341](nodejs/node#36341)
-   \[[`004eafbebf`](nodejs/node@004eafbebf)] - **(SEMVER-MINOR)** **lib**: add unsubscribe method to non-active DC channels (simon-id) [#&#8203;40433](nodejs/node#40433)
-   \[[`625be7585d`](nodejs/node@625be7585d)] - **(SEMVER-MINOR)** **lib**: add return value for DC channel.unsubscribe (simon-id) [#&#8203;40433](nodejs/node#40433)
-   \[[`607bc74eae`](nodejs/node@607bc74eae)] - **(SEMVER-MINOR)** **module**: support pattern trailers (Guy Bedford) [#&#8203;39635](nodejs/node#39635)
-   \[[`f74fe2a59c`](nodejs/node@f74fe2a59c)] - **(SEMVER-MINOR)** **src**: make napi_create_reference accept symbol (JckXia) [#&#8203;39926](nodejs/node#39926)

##### Commits

-   \[[`0231ffa501`](nodejs/node@0231ffa501)] - **build**: add `--without-corepack` (Jonah Snider) [#&#8203;41060](nodejs/node#41060)
-   \[[`5389b8ab05`](nodejs/node@5389b8ab05)] - **crypto**: update root certificates (Richard Lau) [#&#8203;40280](nodejs/node#40280)
-   \[[`0d448eaab5`](nodejs/node@0d448eaab5)] - **(SEMVER-MINOR)** **crypto**: make FIPS related options always available (Vít Ondruch) [#&#8203;36341](nodejs/node#36341)
-   \[[`cd20ecc7cb`](nodejs/node@cd20ecc7cb)] - **deps**: upgrade Corepack to 0.10 (Maël Nison) [#&#8203;40374](nodejs/node#40374)
-   \[[`737df75e17`](nodejs/node@737df75e17)] - **(SEMVER-MINOR)** **deps**: add corepack (Maël Nison) [#&#8203;39608](nodejs/node#39608)
-   \[[`b85aa5a143`](nodejs/node@b85aa5a143)] - **deps**: upgrade npm to 6.14.16 (Ruy Adorno) [#&#8203;41603](nodejs/node#41603)
-   \[[`2755d391a5`](nodejs/node@2755d391a5)] - **deps**: update ICU to 70.1 (Michaël Zasso) [#&#8203;40658](nodejs/node#40658)
-   \[[`3089326d89`](nodejs/node@3089326d89)] - **deps**: update archs files for OpenSSL-1.1.1m (Richard Lau) [#&#8203;41173](nodejs/node#41173)
-   \[[`59da7c12aa`](nodejs/node@59da7c12aa)] - **deps**: upgrade openssl sources to 1.1.1m (Richard Lau) [#&#8203;41173](nodejs/node#41173)
-   \[[`cede1f26f6`](nodejs/node@cede1f26f6)] - **deps**: add -fno-strict-aliasing flag to libuv (Daniel Bevenius) [#&#8203;40631](nodejs/node#40631)
-   \[[`4477da858f`](nodejs/node@4477da858f)] - **doc**: fix corepack grammar for `--force` flag (Steven) [#&#8203;40762](nodejs/node#40762)
-   \[[`5971d58600`](nodejs/node@5971d58600)] - **doc**: add missing YAML tag in `esm.md` (Antoine du Hamel) [#&#8203;41516](nodejs/node#41516)
-   \[[`e903798ae1`](nodejs/node@e903798ae1)] - **doc**: add note regarding unfinished TLA (Antoine du Hamel) [#&#8203;41434](nodejs/node#41434)
-   \[[`a90defebcf`](nodejs/node@a90defebcf)] - **esm**: make `process.exit()` default to exit code 0 (Gang Chen) [#&#8203;41388](nodejs/node#41388)
-   \[[`fc328f1ab0`](nodejs/node@fc328f1ab0)] - **fs**: nullish coalescing to respect zero positional reads (Omar El-Mihilmy) [#&#8203;40716](nodejs/node#40716)
-   \[[`004eafbebf`](nodejs/node@004eafbebf)] - **(SEMVER-MINOR)** **lib**: add unsubscribe method to non-active DC channels (simon-id) [#&#8203;40433](nodejs/node#40433)
-   \[[`625be7585d`](nodejs/node@625be7585d)] - **(SEMVER-MINOR)** **lib**: add return value for DC channel.unsubscribe (simon-id) [#&#8203;40433](nodejs/node#40433)
-   \[[`2c365961d0`](nodejs/node@2c365961d0)] - **module**: support pattern trailers for imports field (Guy Bedford) [#&#8203;40041](nodejs/node#40041)
-   \[[`607bc74eae`](nodejs/node@607bc74eae)] - **(SEMVER-MINOR)** **module**: support pattern trailers (Guy Bedford) [#&#8203;39635](nodejs/node#39635)
-   \[[`f74fe2a59c`](nodejs/node@f74fe2a59c)] - **(SEMVER-MINOR)** **src**: make napi_create_reference accept symbol (JckXia) [#&#8203;39926](nodejs/node#39926)
-   \[[`b050c65885`](nodejs/node@b050c65885)] - **src**: add option to disable loading native addons (Dominic Elm) [#&#8203;39977](nodejs/node#39977)
-   \[[`c1695ac68a`](nodejs/node@c1695ac68a)] - **tools**: update certdata.txt (Richard Lau) [#&#8203;40280](nodejs/node#40280)

</details>

---

### Configuration

📅 **Schedule**: At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Reviewed-on: https://git.walbeck.it/mwalbeck/docker-jellyfin-livestream/pulls/97
Co-authored-by: renovate-bot <bot@walbeck.it>
Co-committed-by: renovate-bot <bot@walbeck.it>
ljharb added a commit to inspect-js/has-package-exports that referenced this pull request Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants