-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Conversation
83ccf1a
to
c638838
Compare
@nodejs/modules |
What will happen to a package using pattern trailers, in a version of node that supports subpath patterns but not pattern trailers? |
@ljharb it would simply be ignored as was reserved for the forwards compatibility path here. In the above example, |
Great! It’s a shame this wasn’t in the initial version, but it’s a great improvement now. |
Before this lands, is there any remotely easy way i can test it locally against my package being imported by a dependency? |
@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. |
Test build: https://ci-release.nodejs.org/job/iojs+release/7075/ I'll update this message when it's available to download. |
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? |
Download links: https://nodejs.org/download/test/v17.0.0-test24288e0c26/ |
Ping for further reviewers here //cc @nodejs/modules. |
@targos unfortunately i can't easily install that with nvm because it's not present in the |
There was a problem hiding this 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.
@guybedford it'd be great if i could test this before you land it; or at least before it's backported. |
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. |
@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) |
@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. |
@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 :-/ |
@MylesBorins as far back as we can go yes, 14 could be fine though. |
I got it downloaded and installed by locally hardcoding nvm to accept 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:
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? |
@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. |
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. |
The best option is yes to use the extensioned form, and then use the exports field:
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:
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 |
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
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
PR-URL: nodejs#39635 Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
PR-URL: #39635 Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
PR-URL: #39635 Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
PR-URL: #39635 Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
PR-URL: nodejs#39635 Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
PR-URL: #39635 Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
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
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
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 - [#​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 - [#​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 - [#​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 - [#​40280](nodejs/node#40280) ##### Other Notable Changes - \[[`0d448eaab5`](nodejs/node@0d448eaab5)] - **(SEMVER-MINOR)** **crypto**: make FIPS related options always available (Vít Ondruch) [#​36341](nodejs/node#36341) - \[[`004eafbebf`](nodejs/node@004eafbebf)] - **(SEMVER-MINOR)** **lib**: add unsubscribe method to non-active DC channels (simon-id) [#​40433](nodejs/node#40433) - \[[`625be7585d`](nodejs/node@625be7585d)] - **(SEMVER-MINOR)** **lib**: add return value for DC channel.unsubscribe (simon-id) [#​40433](nodejs/node#40433) - \[[`607bc74eae`](nodejs/node@607bc74eae)] - **(SEMVER-MINOR)** **module**: support pattern trailers (Guy Bedford) [#​39635](nodejs/node#39635) - \[[`f74fe2a59c`](nodejs/node@f74fe2a59c)] - **(SEMVER-MINOR)** **src**: make napi_create_reference accept symbol (JckXia) [#​39926](nodejs/node#39926) ##### Commits - \[[`0231ffa501`](nodejs/node@0231ffa501)] - **build**: add `--without-corepack` (Jonah Snider) [#​41060](nodejs/node#41060) - \[[`5389b8ab05`](nodejs/node@5389b8ab05)] - **crypto**: update root certificates (Richard Lau) [#​40280](nodejs/node#40280) - \[[`0d448eaab5`](nodejs/node@0d448eaab5)] - **(SEMVER-MINOR)** **crypto**: make FIPS related options always available (Vít Ondruch) [#​36341](nodejs/node#36341) - \[[`cd20ecc7cb`](nodejs/node@cd20ecc7cb)] - **deps**: upgrade Corepack to 0.10 (Maël Nison) [#​40374](nodejs/node#40374) - \[[`737df75e17`](nodejs/node@737df75e17)] - **(SEMVER-MINOR)** **deps**: add corepack (Maël Nison) [#​39608](nodejs/node#39608) - \[[`b85aa5a143`](nodejs/node@b85aa5a143)] - **deps**: upgrade npm to 6.14.16 (Ruy Adorno) [#​41603](nodejs/node#41603) - \[[`2755d391a5`](nodejs/node@2755d391a5)] - **deps**: update ICU to 70.1 (Michaël Zasso) [#​40658](nodejs/node#40658) - \[[`3089326d89`](nodejs/node@3089326d89)] - **deps**: update archs files for OpenSSL-1.1.1m (Richard Lau) [#​41173](nodejs/node#41173) - \[[`59da7c12aa`](nodejs/node@59da7c12aa)] - **deps**: upgrade openssl sources to 1.1.1m (Richard Lau) [#​41173](nodejs/node#41173) - \[[`cede1f26f6`](nodejs/node@cede1f26f6)] - **deps**: add -fno-strict-aliasing flag to libuv (Daniel Bevenius) [#​40631](nodejs/node#40631) - \[[`4477da858f`](nodejs/node@4477da858f)] - **doc**: fix corepack grammar for `--force` flag (Steven) [#​40762](nodejs/node#40762) - \[[`5971d58600`](nodejs/node@5971d58600)] - **doc**: add missing YAML tag in `esm.md` (Antoine du Hamel) [#​41516](nodejs/node#41516) - \[[`e903798ae1`](nodejs/node@e903798ae1)] - **doc**: add note regarding unfinished TLA (Antoine du Hamel) [#​41434](nodejs/node#41434) - \[[`a90defebcf`](nodejs/node@a90defebcf)] - **esm**: make `process.exit()` default to exit code 0 (Gang Chen) [#​41388](nodejs/node#41388) - \[[`fc328f1ab0`](nodejs/node@fc328f1ab0)] - **fs**: nullish coalescing to respect zero positional reads (Omar El-Mihilmy) [#​40716](nodejs/node#40716) - \[[`004eafbebf`](nodejs/node@004eafbebf)] - **(SEMVER-MINOR)** **lib**: add unsubscribe method to non-active DC channels (simon-id) [#​40433](nodejs/node#40433) - \[[`625be7585d`](nodejs/node@625be7585d)] - **(SEMVER-MINOR)** **lib**: add return value for DC channel.unsubscribe (simon-id) [#​40433](nodejs/node#40433) - \[[`2c365961d0`](nodejs/node@2c365961d0)] - **module**: support pattern trailers for imports field (Guy Bedford) [#​40041](nodejs/node#40041) - \[[`607bc74eae`](nodejs/node@607bc74eae)] - **(SEMVER-MINOR)** **module**: support pattern trailers (Guy Bedford) [#​39635](nodejs/node#39635) - \[[`f74fe2a59c`](nodejs/node@f74fe2a59c)] - **(SEMVER-MINOR)** **src**: make napi_create_reference accept symbol (JckXia) [#​39926](nodejs/node#39926) - \[[`b050c65885`](nodejs/node@b050c65885)] - **src**: add option to disable loading native addons (Dominic Elm) [#​39977](nodejs/node#39977) - \[[`c1695ac68a`](nodejs/node@c1695ac68a)] - **tools**: update certdata.txt (Richard Lau) [#​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>
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:
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:
"/"
exports that resolve to files will now give the trailing "/" deprecation warning."./x/"
and"./x*"
, the second pattern will take precedence over the path for resolving./x/y
according to the specificity rules.*
can't be matched in patterns, so given"exports": { "./*/*.js": "./*.js" }
this will no longer matchimport './*/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.