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.

Sign up for GitHub

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

Jump to bottom

[12.x] modules backports #35385

Closed
wants to merge 13 commits into from
Closed

[12.x] modules backports #35385

wants to merge 13 commits into from

Conversation

guybedford
Copy link
Contributor

This includes the latest modules backports from 14 to 12, which should then ensure that #35249 can land cleanly as well while getting the latest support for the modules package.json fields for compatibility.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included

guybedford and others added 13 commits September 27, 2020 19:41
@guybedford
module: package "imports" field
75a2cf3 
PR-URL: nodejs#34117
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
@dnlup @guybedford
module: self referential modules in repl or -r
f3a328c 
Load self referential modules from the repl and using the preload flag
`-r`. In both cases the base path used for resolution is the current
`process.cwd()`. Also fixes an internal cycle bug in the REPL exports
resolution.

PR-URL: nodejs#32261
Fixes: nodejs#31595
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
@guybedford
module: use cjsCache over esm injection
39a42b3 
PR-URL: nodejs#34605
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@guybedford
module: custom --conditions flag option
9485ad6 
PR-URL: nodejs#34637
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
@guybedford
module: share CJS/ESM resolver fns, refactoring
7e20df2 
PR-URL: nodejs#34744
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@DerekNonGeneric @guybedford
module: fix check for package.json at volume root
d6af3ba 
This patch converts the "read package scope" algorithm's while loop
into a do-while loop enabling items at the filesystem root dir to
be considered within the scope of a sibling package.json also at the
filesystem root dir.

Fixes: nodejs#33438

Co-authored-by: Guy Bedford <[email protected]>

PR-URL: nodejs#34595
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Mary Marchini <[email protected]>
@richardlau @guybedford
module: drop -u alias for --conditions
56393c1 
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 <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
@lundibundi @guybedford
esm: improve error message of ERR_UNSUPPORTED_ESM_URL_SCHEME
8030eb5 
Refs: nodejs#34765

PR-URL: nodejs#34795
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Trott @guybedford
esm: shorten ERR_UNSUPPORTED_ESM_URL_SCHEME message
96325b1 
I know it just got modified to include new information, but this
shortens the message a bit without (I hope) losing clarity or meaning.

PR-URL: nodejs#34836
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
@guybedford
esm: better package.json parser errors
b29b966 
PR-URL: nodejs#35117
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@aduh95 @guybedford
module: use isURLInstance instead of instanceof
be78b83 
PR-URL: nodejs#34951
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Derek Lewis <[email protected]>
@ctavan @guybedford
module: fix crash on multiline named cjs imports
0772b5b 
The node process crashes when trying to parse a multiline import
statement for named exports of a CommonJS module:

    TypeError: Cannot read property '0' of null
          at ModuleJob._instantiate (internal/modules/esm/module_job.js:112:77)
          at async ModuleJob.run (internal/modules/esm/module_job.js:137:5)
          at async Loader.import (internal/modules/esm/loader.js:165:24)
          at async rejects.name (file:///***/node/test/es-module/test-esm-cjs-named-error.mjs:56:3)
          at async waitForActual (assert.js:721:5)
          at async rejects (assert.js:830:25),

The reason is that the regexp that is currently used to decorate the
original error fails for multi line import statements.

Unfortunately the undecorated error stack only contains the single line
which causes the import to fail:

    file:///***/node/test/fixtures/es-modules/package-cjs-named-error/multi-line.mjs:2
      comeOn,
      ^^^^^^
    SyntaxError: The requested module './fail.cjs' does not provide an export named 'comeOn'
        at ModuleJob._instantiate (internal/modules/esm/module_job.js:98:21)
        at async ModuleJob.run (internal/modules/esm/module_job.js:141:5)
        at async Loader.import (internal/modules/esm/loader.js:165:24)
        at async rejects.name (file:///***/node/test/es-module/test-esm-cjs-named-error.mjs:56:3)
        at async waitForActual (assert.js:721:5)
        at async rejects (assert.js:830:25)

Hence, for multiline import statements we cannot create an equivalent
piece of code that uses default import followed by an object
destructuring assignment.

In any case the node process should definitely not crash. So until we
have a more sophisticated way of extracting the entire problematic
multiline import statement, show the code example only for single-line
imports where the current regexp approach works well.

Refs: nodejs#35259

PR-URL: nodejs#35275
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@guybedford
module: exports pattern support
ab60e95 
PR-URL: nodejs#34718
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. v12.x labels Sep 28, 2020
@guybedford guybedford mentioned this pull request Sep 28, 2020
Closed
4 tasks
@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 28, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 28, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@addaleax addaleax added the esm Issues and PRs related to the ECMAScript Modules implementation. label Sep 28, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@guybedford guybedford mentioned this pull request Sep 29, 2020
Closed
4 tasks
jkrems
jkrems approved these changes Oct 1, 2020
View reviewed changes
Copy link
Contributor

@jkrems jkrems left a comment

Choose a reason for hiding this comment

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

RSLGTM (skimmed the changes and they look like I'd expect)

@codebytere
Copy link
Member

Landed in b7be751...2f3ffc0

@codebytere codebytere closed this Oct 1, 2020
codebytere pushed a commit that referenced this pull request Oct 1, 2020
@guybedford @codebytere
module: package "imports" field
d065334 
PR-URL: #34117
Backport-PR-URL: #35385
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
codebytere pushed a commit that referenced this pull request Oct 1, 2020
@dnlup @codebytere
module: self referential modules in repl or -r
00aa935 
Load self referential modules from the repl and using the preload flag
`-r`. In both cases the base path used for resolution is the current
`process.cwd()`. Also fixes an internal cycle bug in the REPL exports
resolution.

PR-URL: #32261
Backport-PR-URL: #35385
Fixes: #31595
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
codebytere pushed a commit that referenced this pull request Oct 1, 2020
@guybedford @codebytere
module: use cjsCache over esm injection
3ad146d 
PR-URL: #34605
Backport-PR-URL: #35385
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
codebytere pushed a commit that referenced this pull request Oct 1, 2020
@guybedford @codebytere
module: custom --conditions flag option
d9857fd 
PR-URL: #34637
Backport-PR-URL: #35385
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
codebytere pushed a commit that referenced this pull request Oct 1, 2020
@guybedford @codebytere
module: share CJS/ESM resolver fns, refactoring
cecc193 
PR-URL: #34744
Backport-PR-URL: #35385
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this pull request Oct 1, 2020
@codebytere
module: fix check for package.json at volume root
740c958 
This patch converts the "read package scope" algorithm's while loop
into a do-while loop enabling items at the filesystem root dir to
be considered within the scope of a sibling package.json also at the
filesystem root dir.

Fixes: #33438

Co-authored-by: Guy Bedford <[email protected]>

PR-URL: #34595
Backport-PR-URL: #35385
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Mary Marchini <[email protected]>
codebytere pushed a commit that referenced this pull request Oct 1, 2020
@richardlau @codebytere
module: drop -u alias for --conditions
fc93cc9 
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 <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
codebytere pushed a commit that referenced this pull request Oct 1, 2020
@lundibundi @codebytere
esm: improve error message of ERR_UNSUPPORTED_ESM_URL_SCHEME
551be2a 
Refs: #34765

PR-URL: #34795
Backport-PR-URL: #35385
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
codebytere pushed a commit that referenced this pull request Oct 1, 2020
@Trott @codebytere
esm: shorten ERR_UNSUPPORTED_ESM_URL_SCHEME message
9159649 
I know it just got modified to include new information, but this
shortens the message a bit without (I hope) losing clarity or meaning.

PR-URL: #34836
Backport-PR-URL: #35385
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
codebytere pushed a commit that referenced this pull request Oct 1, 2020
@guybedford @codebytere
esm: better package.json parser errors
bf7b796 
PR-URL: #35117
Backport-PR-URL: #35385
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
codebytere pushed a commit that referenced this pull request Oct 1, 2020
@aduh95 @codebytere
module: use isURLInstance instead of instanceof
89a58f6 
PR-URL: #34951
Backport-PR-URL: #35385
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Derek Lewis <[email protected]>
codebytere pushed a commit that referenced this pull request Oct 1, 2020
@ctavan @codebytere
module: fix crash on multiline named cjs imports
ed3278d 
The node process crashes when trying to parse a multiline import
statement for named exports of a CommonJS module:

    TypeError: Cannot read property '0' of null
          at ModuleJob._instantiate (internal/modules/esm/module_job.js:112:77)
          at async ModuleJob.run (internal/modules/esm/module_job.js:137:5)
          at async Loader.import (internal/modules/esm/loader.js:165:24)
          at async rejects.name (file:///***/node/test/es-module/test-esm-cjs-named-error.mjs:56:3)
          at async waitForActual (assert.js:721:5)
          at async rejects (assert.js:830:25),

The reason is that the regexp that is currently used to decorate the
original error fails for multi line import statements.

Unfortunately the undecorated error stack only contains the single line
which causes the import to fail:

    file:///***/node/test/fixtures/es-modules/package-cjs-named-error/multi-line.mjs:2
      comeOn,
      ^^^^^^
    SyntaxError: The requested module './fail.cjs' does not provide an export named 'comeOn'
        at ModuleJob._instantiate (internal/modules/esm/module_job.js:98:21)
        at async ModuleJob.run (internal/modules/esm/module_job.js:141:5)
        at async Loader.import (internal/modules/esm/loader.js:165:24)
        at async rejects.name (file:///***/node/test/es-module/test-esm-cjs-named-error.mjs:56:3)
        at async waitForActual (assert.js:721:5)
        at async rejects (assert.js:830:25)

Hence, for multiline import statements we cannot create an equivalent
piece of code that uses default import followed by an object
destructuring assignment.

In any case the node process should definitely not crash. So until we
have a more sophisticated way of extracting the entire problematic
multiline import statement, show the code example only for single-line
imports where the current regexp approach works well.

Refs: #35259

PR-URL: #35275
Backport-PR-URL: #35385
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
codebytere pushed a commit that referenced this pull request Oct 1, 2020
@guybedford @codebytere
module: exports pattern support
2f3ffc0 
PR-URL: #34718
Backport-PR-URL: #35385
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@guybedford guybedford mentioned this pull request Oct 22, 2020
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkrems jkrems jkrems approved these changes

@codebytere codebytere codebytere approved these changes

Assignees
No one assigned
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.