module: custom --conditions flag option #34637

guybedford · 2020-08-05T19:08:22Z

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

lib/internal/modules/cjs/loader.js

src/node_options.cc

lib/internal/modules/esm/resolve.js

doc/api/esm.md

bmeck · 2020-08-05T21:03:30Z

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

doc/api/cli.md

doc/api/esm.md

lib/internal/modules/cjs/loader.js

lib/internal/modules/esm/resolve.js

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

doc/api/cli.md

guybedford · 2020-08-05T21:32:26Z

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?

bmeck · 2020-08-05T21:36:37Z

@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.

guybedford · 2020-08-05T21:49:54Z

@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.

ljharb · 2020-08-06T00:00:47Z

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

doc/api/cli.md

bmeck · 2020-08-06T14:25:54Z

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.

ljharb · 2020-08-06T17:19:20Z

@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?

bmeck · 2020-08-06T17:21:04Z

@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.

GeoffreyBooth · 2020-08-06T17:46:13Z

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.

bmeck · 2020-08-06T18:20:29Z

@GeoffreyBooth agreed

nodejs-github-bot · 2020-08-08T16:36:47Z

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>

guybedford · 2020-08-11T21:36:15Z

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>

addaleax · 2020-09-26T13:22:39Z

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

guybedford · 2020-09-27T01:43:59Z

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>

guybedford · 2020-09-28T03:49:55Z

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

lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved

fixup resolution bugs

5dc585d

bmeck reviewed Aug 5, 2020

View reviewed changes

src/node_options.cc Outdated Show resolved Hide resolved

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

jkrems reviewed Aug 5, 2020

View reviewed changes

src/node_options.cc Outdated Show resolved Hide resolved

lib/internal/modules/esm/resolve.js Show resolved Hide resolved

doc/api/esm.md Outdated Show resolved Hide resolved

ljharb reviewed Aug 5, 2020

View reviewed changes

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

doc/api/cli.md Outdated Show resolved Hide resolved

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

doc/api/cli.md Show resolved Hide resolved

GeoffreyBooth approved these changes Aug 6, 2020

View reviewed changes

jkrems approved these changes Aug 6, 2020

View reviewed changes

frehner mentioned this pull request Aug 11, 2020

Formalize top-level ES exports facebook/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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: custom --conditions flag option #34637

module: custom --conditions flag option #34637

guybedford commented Aug 5, 2020 •

edited

Loading

bmeck commented Aug 5, 2020

guybedford commented Aug 5, 2020

bmeck commented Aug 5, 2020

guybedford commented Aug 5, 2020

ljharb commented Aug 6, 2020

bmeck commented Aug 6, 2020

ljharb commented Aug 6, 2020

bmeck commented Aug 6, 2020

GeoffreyBooth commented Aug 6, 2020

bmeck commented Aug 6, 2020

nodejs-github-bot commented Aug 8, 2020

guybedford commented Aug 11, 2020

addaleax commented Sep 26, 2020

guybedford commented Sep 27, 2020 •

edited

Loading

guybedford commented Sep 28, 2020

module: custom --conditions flag option #34637

module: custom --conditions flag option #34637

Conversation

guybedford commented Aug 5, 2020 • edited Loading

Checklist

bmeck commented Aug 5, 2020

guybedford commented Aug 5, 2020

bmeck commented Aug 5, 2020

guybedford commented Aug 5, 2020

ljharb commented Aug 6, 2020

bmeck commented Aug 6, 2020

ljharb commented Aug 6, 2020

bmeck commented Aug 6, 2020

GeoffreyBooth commented Aug 6, 2020

bmeck commented Aug 6, 2020

nodejs-github-bot commented Aug 8, 2020

guybedford commented Aug 11, 2020

addaleax commented Sep 26, 2020

guybedford commented Sep 27, 2020 • edited Loading

guybedford commented Sep 28, 2020

guybedford commented Aug 5, 2020 •

edited

Loading

guybedford commented Sep 27, 2020 •

edited

Loading