module: custom --conditions flag option by guybedford · Pull Request #34637 · nodejs/node · GitHub

This implements a node --conditions=custom flag for supporting custom resolution conditions in conditional exports, as previously discussed in nodejs/modules#537. A short alias variant, -u=custom is also provided, which stands for "user conditions".

Any custom condition names can be provided as repeated flags, but the core node, default, require and import conditions cannot be changed.

In the process a couple of conditional resolution edge ase bugs are fixed as well here.

@nodejs/modules-active-members

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

we likely should ban known contextual conditions import and require from coming from the CLI

Co-authored-by: Jordan Harband <ljharb@gmail.com> Co-authored-by: Jan Olaf Krems <jan.krems@gmail.com>

we likely should ban known contextual conditions import and require from coming from the CLI

I was thinking about this case and the worst case seems to be having CJS resolve ESM or ESM resolve the CJS path depending on how the ordering works out.

Neither of these seem that bad to me though if users want to do it?

@guybedford I agree it isn't fatal in any way if users want to set those for w/e reason, i just have concerns since we have statements in the docs about being exclusive being invalidated.

@bmeck yes worth thinking about, although the main property behind the exclusive definition was that one or the other will always be matched (so there are no fallbacks when both are set in any environment), but that property is retained.

Given that ESM and CJS are ambiguous, it seems like a very not good idea to allow accidental mis-parsing.

Given that ESM and CJS are ambiguous, it seems like a very not good idea to allow accidental mis-parsing.

The conditions do not affect the static format of a file and even if you set the require condition and target a .mjs file it is still ESM and fails and vice versa if you set the import condition and target a .cjs file it is still seen as CJS.

@bmeck so to confirm, you're saying even if i have a file that contains nothing but console.log('parsed'), there's no way (as a CJS file extension) it can be parsed as a Module, and no way (as an ESM file extension) it can be parsed as a Script?

@ljharb not via this PR, I might be able to fiddle with the old CJS loader to get it to parse wrong with effort; but to my knowledge it is never possible with the ESM loader.

I might be able to fiddle with the old CJS loader to get it to parse wrong with effort

I'm pretty sure this should be impossible, at least for accidental cases, since we made require of .mjs throw. It should be likewise for require of .js under "type": "module". I'm sure there are hacky ways to circumvent our check like data URIs or extensionless files or something, but for users who aren't trying to trick Node we should already be erroring.

@GeoffreyBooth agreed

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

PR-URL: #34637 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com> Reviewed-By: Jan Krems <jan.krems@gmail.com>

Landed in 77a515c.

PR-URL: #34637 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com> Reviewed-By: Jan Krems <jan.krems@gmail.com>

Old versions of mocha break after nodejs#34637. This was a bug in mocha, but since this is a widely used module we can expect ecosystem breakage until modules are updated to the latest version of mocha. Drop the conflicting `-u` alias -- we can potentially bring it back once modules have been updated.

Old versions of mocha break after #34637. This was a bug in mocha, but since this is a widely used module we can expect ecosystem breakage until modules are updated to the latest version of mocha. Drop the conflicting `-u` alias -- we can potentially bring it back once modules have been updated. PR-URL: #34935 Refs: mochajs/mocha#4417 Refs: #34637 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Shelley Vohr <codebytere@gmail.com>

If this is backported to v12.x, it should come together with #34935

Thanks, this likely should be backported, if not for any other reason than to simplify the backporting of subsequent modules PRs.

PR-URL: nodejs#34637 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com> Reviewed-By: Jan Krems <jan.krems@gmail.com>

Old versions of mocha break after nodejs#34637. This was a bug in mocha, but since this is a widely used module we can expect ecosystem breakage until modules are updated to the latest version of mocha. Drop the conflicting `-u` alias -- we can potentially bring it back once modules have been updated. PR-URL: nodejs#34935 Refs: mochajs/mocha#4417 Refs: nodejs#34637 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Shelley Vohr <codebytere@gmail.com>

This and #34935 are included in the backport PR at #35385.

PR-URL: #34637 Backport-PR-URL: #35385 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com> Reviewed-By: Jan Krems <jan.krems@gmail.com>

Old versions of mocha break after #34637. This was a bug in mocha, but since this is a widely used module we can expect ecosystem breakage until modules are updated to the latest version of mocha. Drop the conflicting `-u` alias -- we can potentially bring it back once modules have been updated. PR-URL: #34935 Backport-PR-URL: #35385 Refs: mochajs/mocha#4417 Refs: #34637 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Shelley Vohr <codebytere@gmail.com>

module: custom --conditions flag option

11928c7

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation. labels Aug 5, 2020

bmeck reviewed Aug 5, 2020

View reviewed changes

Comment thread lib/internal/modules/cjs/loader.js Outdated

fixup resolution bugs

5dc585d

bmeck reviewed Aug 5, 2020

View reviewed changes

Comment thread src/node_options.cc Outdated

guybedford added 6 commits August 5, 2020 12:49

remove condition alias for now

936a213

todo is done

e1a91c8

-m alias, docs

ed69235

doc reorder

0180f86

rename heading

dba70c1

use -u short flag

52f216c

hybrist reviewed Aug 5, 2020

View reviewed changes

Comment thread src/node_options.cc Outdated

Comment thread lib/internal/modules/esm/resolve.js

Comment thread doc/api/esm.md Outdated

ljharb reviewed Aug 5, 2020

View reviewed changes

Comment thread doc/api/cli.md Outdated

Comment thread doc/api/esm.md Outdated

Comment thread doc/api/esm.md Outdated

Comment thread lib/internal/modules/cjs/loader.js Outdated

Comment thread lib/internal/modules/esm/resolve.js Outdated

Apply suggestions from code review

acbe3ee

Co-authored-by: Jordan Harband <ljharb@gmail.com> Co-authored-by: Jan Olaf Krems <jan.krems@gmail.com>

guybedford mentioned this pull request Aug 5, 2020

cli: --dev flag for development exports resolution condition #33171

Closed

4 tasks

addaleax reviewed Aug 5, 2020

View reviewed changes

Comment thread doc/api/cli.md Outdated

use multiple flags over comma separated

1ed44a4

addaleax approved these changes Aug 5, 2020

View reviewed changes

GeoffreyBooth reviewed Aug 6, 2020

View reviewed changes

Comment thread doc/api/cli.md

GeoffreyBooth approved these changes Aug 6, 2020

View reviewed changes

hybrist approved these changes Aug 6, 2020

View reviewed changes

frehner mentioned this pull request Aug 11, 2020

Formalize top-level ES exports react/react#11503

Open

guybedford added a commit that referenced this pull request Aug 11, 2020

module: custom --conditions flag option

77a515c

PR-URL: #34637 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com> Reviewed-By: Jan Krems <jan.krems@gmail.com>

guybedford closed this Aug 11, 2020

MylesBorins pushed a commit that referenced this pull request Aug 17, 2020

module: custom --conditions flag option

4831a84

PR-URL: #34637 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com> Reviewed-By: Jan Krems <jan.krems@gmail.com>

danielleadams mentioned this pull request Aug 20, 2020

V14.9.0 proposal #34852

Merged

BethGriggs pushed a commit that referenced this pull request Aug 20, 2020

module: custom --conditions flag option

6929649

PR-URL: #34637 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com> Reviewed-By: Jan Krems <jan.krems@gmail.com>

wclr mentioned this pull request Aug 24, 2020

Add conditions to require.resolve options #34901

Closed

boneskull mentioned this pull request Aug 25, 2020

node flag parsing strategy is backwards mochajs/mocha#4417

Closed

richardlau mentioned this pull request Aug 26, 2020

module: drop -u alias for --conditions #34935

Closed

4 tasks

addaleax added the backport-requested-v12.x label Sep 26, 2020

addaleax added backport-open-v12.x and removed backport-requested-v12.x labels Sep 28, 2020

codebytere mentioned this pull request Oct 4, 2020

v12.19.0 proposal #35401

Merged

agilgur5 mentioned this pull request Oct 12, 2020

Use ESM build in modern browser (<script type="module" ...>) jaredpalmer/tsdx#631

Closed

aduh95 mentioned this pull request Feb 9, 2021

doc: add version metadata for packages features #37289

Merged

richardlau mentioned this pull request May 21, 2021

cli: -C alias for --conditions flag #38755

Closed

module: custom --conditions flag option by guybedford · Pull Request #34637 · nodejs/node · GitHub

Uh oh!

Uh oh!

module: custom --conditions flag option - #34637

module: custom --conditions flag option#34637guybedford wants to merge 10 commits intonodejs:masternodejs/node:masterfrom guybedford:conditions-flagguybedford/node:conditions-flagCopy head branch name to clipboard

Conversation

guybedford commented Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bmeck commented Aug 5, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guybedford commented Aug 5, 2020

Uh oh!

bmeck commented Aug 5, 2020

Uh oh!

guybedford commented Aug 5, 2020

Uh oh!

ljharb commented Aug 6, 2020

Uh oh!

Uh oh!

bmeck commented Aug 6, 2020

Uh oh!

ljharb commented Aug 6, 2020

Uh oh!

bmeck commented Aug 6, 2020

Uh oh!

GeoffreyBooth commented Aug 6, 2020

Uh oh!

bmeck commented Aug 6, 2020

Uh oh!

nodejs-github-bot commented Aug 8, 2020

Uh oh!

guybedford commented Aug 11, 2020

Uh oh!

addaleax commented Sep 26, 2020

Uh oh!

guybedford commented Sep 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guybedford commented Sep 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

module: custom --conditions flag option#34637
guybedford wants to merge 10 commits into
nodejs:masterfrom
guybedford:conditions-flag

guybedford commented Aug 5, 2020 •
edited
Loading

guybedford commented Sep 27, 2020 •
edited
Loading