esm: merge esm and cjs package.json caches by shackijj · Pull Request #33229 · nodejs/node

shackijj · 2020-05-04T10:33:10Z

Relates to: #30674

Checklist

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

shackijj · 2020-05-04T10:40:13Z

@guybedford as discussed earlier in the issue, I tried to keep diffs down.
As the next step, we could move readPackageScope, readPackage, packageJsonCache functions from cjs loader to a separate module. The module will be used by esm and cjs loaders.

Additionally, I'd move the following checks to readPackage function (previously they were in getPackageConfig in esm loader).

  if (typeof main !== 'string') main = undefined;	
  if (typeof name !== 'string') name = undefined;	
  // Ignore unknown types for forwards compatibility	
  if (type !== 'module' && type !== 'commonjs') type = 'none'

shackijj · 2020-05-04T17:19:23Z

One test failed on MacOS (Build from tarball / test-tarball-macOS).

not ok 2356 parallel/test-worker-init-failure
  ---
  duration_ms: 2.77
  severity: fail
  exitcode: 1
  stack: |-
    child stdout: 
    
    child stderr: assert.js:920
        throw err;
        ^
    
    AssertionError [ERR_ASSERTION]: The input did not match the regular expression /EMFILE/. Input:
    
    'ENOENT: no such file or directory, uv_cwd'
    
        at Worker.<anonymous> (/Users/runner/runners/2.169.1/work/node/node/node-v15.0.0-nightly2020-05-04xxxx/test/parallel/test-worker-init-failure.js:38:14)
        at Worker.emit (events.js:315:20)
        at Worker.[kOnErrorMessage] (internal/worker.js:233:10)
        at Worker.[kOnMessage] (internal/worker.js:243:37)
        at MessagePort.<anonymous> (internal/worker.js:164:57)
        at MessagePort.emit (events.js:315:20)
        at MessagePort.onmessage (internal/worker/io.js:78:8)
        at MessagePort.exports.emitMessage (internal/per_context/messageport.js:11:10)

It seems that the test does not relate the changes I made. Also, this job finished succefully (same MacOS environment). Can't reproduce this on my Mac as well.
test/parallel/test-worker-init-failure.js checks extreme scenario by creating 256 workers.

Is there a chance that the test is flaky? Is there a way to restrart ci pipeline without a new commit?

@guybedford do you have any thought on that?

guybedford

Thanks for keeping the diff manageable here, makes a big difference and it's looking really nice and simple.

We should just check the error / missing package.json cases carefully here, but this is looking almost good to go.

lib/internal/modules/esm/resolve.js

guybedford · 2020-05-04T20:12:25Z

@shackijj those further refactorings sound really sensible to me too, if you want to add them in this PR too feel free.

shackijj · 2020-05-06T08:46:35Z

@guybedford I decided not to move the functions to a separate module until you approve the last changes that I made.

Added test for ERR_INVALID_PACKAGE_CONFIG.

internalModuleReadJSON maps an invalid JSON to a valid JSON (e.g }{ -> {}, {'invalid'} -> {}), so we need a different file reading strategy in ESM loader as to throw ERR_INVALID_PACKAGE_CONFIG.

P.S. I believe we could document internalModuleReadJSON a bit better.

guybedford

@shackijj this is looking really good and thorough thank you. I think we're nearly there.

Yes I think following the clear specification for when to throw invalid makes more sense than the perf optimization for esm. So the branching seems good to me.

lib/internal/modules/esm/resolve.js

guybedford

Looks great to me, and we just got that negative diff!

//cc @nodejs/modules-active-members if anyone else wants to review this PR.

test/es-module/test-esm-invalid-pjson.js

add test for ERR_INVALID_PACKAGE_CONFIG error Refs: nodejs#30674

shackijj · 2020-05-10T09:11:58Z

@guybedford I rebased the branch and changed the history a bit as to follow the commit guideline. Also, the guideline states that I should add 'author ready' label to this PR. It seems that I don't have rights to add a label to PR. Could you please add it?

test/fixtures/node_modules/invalid-pjson/package.json

lib/internal/modules/cjs/loader.js

test/fixtures/node_modules/invalid-pjson/package.json Refs: nodejs#30674

Refs: nodejs#30674

lib/internal/modules/esm/resolve.js

guybedford · 2020-05-18T20:10:07Z

This looks great to me. //cc @addaleax for C++ review.

src/node_file.cc

Refs: nodejs#30674

shackijj · 2020-05-20T11:03:41Z

The tests failed on MacOS with the following error

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ 'ERR_STREAM_DESTROYED'
- 'ECONNRESET'
    at ClientRequest.<anonymous> (/Users/runner/runners/2.262.1/work/node/node/test/parallel/test-http-destroyed-socket-write2.js:66:16)
    at ClientRequest.<anonymous> (/Users/runner/runners/2.262.1/work/node/node/test/common/index.js:363:15)
    at ClientRequest.emit (events.js:315:20)
    at emitErrorNt (_http_outgoing.js:666:43)
    at processTicksAndRejections (internal/process/task_queues.js:85:21) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 'ERR_STREAM_DESTROYED',
  expected: 'ECONNRESET',
  operator: 'strictEqual'
}
Command: out/Release/node /Users/runner/runners/2.262.1/work/node/node/test/parallel/test-http-destroyed-socket-write2.js

It seems that the test is not related this fix and might be flaky.

nodejs-github-bot · 2020-05-20T19:41:41Z

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

GeoffreyBooth · 2020-05-24T04:45:35Z

https://ci.nodejs.org/job/node-test-pull-request/31456/ is green, this can land.

Refs: #30674 PR-URL: #33229 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>

GeoffreyBooth · 2020-05-24T23:24:39Z

Landed in 8f10bb2.

Refs: #30674 PR-URL: #33229 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>

nodejs-github-bot added the esm Issues and PRs related to the ECMAScript Modules implementation. label May 4, 2020

shackijj force-pushed the sharing-package-json-cache-#30674 branch from adcfd60 to c14e769 Compare May 4, 2020 12:55

guybedford reviewed May 4, 2020

View reviewed changes

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

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

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

shackijj force-pushed the sharing-package-json-cache-#30674 branch from e149015 to ce1005a Compare May 6, 2020 08:32

guybedford reviewed May 7, 2020

View reviewed changes

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

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

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

guybedford approved these changes May 8, 2020

View reviewed changes

guybedford reviewed May 9, 2020

View reviewed changes

test/es-module/test-esm-invalid-pjson.js Outdated Show resolved Hide resolved

esm: share package.json cache between ESM and CJS loaders

0227b07

add test for ERR_INVALID_PACKAGE_CONFIG error Refs: nodejs#30674

shackijj force-pushed the sharing-package-json-cache-#30674 branch from 4f19db7 to 0227b07 Compare May 10, 2020 09:07

hybrist reviewed May 10, 2020

View reviewed changes

test/fixtures/node_modules/invalid-pjson/package.json Outdated Show resolved Hide resolved

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

shackijj added 2 commits May 11, 2020 11:00

esm: add a trailing new line.

1d4a52c

test/fixtures/node_modules/invalid-pjson/package.json Refs: nodejs#30674

esm: share package.json string cache between ESM and CJS loaders

3d0ac05

Refs: nodejs#30674

ljharb reviewed May 18, 2020

View reviewed changes

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

esm: add removePackageJsonFromPath

e5f3182

shackijj force-pushed the sharing-package-json-cache-#30674 branch from 2b38d67 to e5f3182 Compare May 18, 2020 08:12

addaleax reviewed May 19, 2020

View reviewed changes

src/node_file.cc Outdated Show resolved Hide resolved

esm: internalModuleReadJSON returns an array

0d947ec

Refs: nodejs#30674

shackijj force-pushed the sharing-package-json-cache-#30674 branch from d760659 to 0d947ec Compare May 20, 2020 08:04

GeoffreyBooth approved these changes May 24, 2020

View reviewed changes

GeoffreyBooth added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 24, 2020

GeoffreyBooth pushed a commit that referenced this pull request May 24, 2020

esm: share package.json cache between ESM and CJS loaders

8f10bb2

Refs: #30674 PR-URL: #33229 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>

GeoffreyBooth removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 24, 2020

GeoffreyBooth closed this May 24, 2020

addaleax mentioned this pull request May 25, 2020

fs: add mkstemp functions #33549

Closed

4 tasks

GeoffreyBooth mentioned this pull request May 28, 2020

[WIP] Support requiring .mjs files #30891

Closed

codebytere pushed a commit that referenced this pull request Jun 18, 2020

esm: share package.json cache between ESM and CJS loaders

5bef20c

Refs: #30674 PR-URL: #33229 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>

codebytere mentioned this pull request Jun 28, 2020

v14.5.0 proposal #34093

Merged

codebytere pushed a commit that referenced this pull request Jun 30, 2020

esm: share package.json cache between ESM and CJS loaders

3d2e7e7

Refs: #30674 PR-URL: #33229 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>

codebytere pushed a commit that referenced this pull request Jul 8, 2020

esm: share package.json cache between ESM and CJS loaders

3559471

Refs: #30674 PR-URL: #33229 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>

codebytere mentioned this pull request Jul 13, 2020

v12.18.3 proposal #34343

Merged

guybedford mentioned this pull request Jul 30, 2020

Sharing package.json and stat cache between CJS and ESM loaders #30674

Closed

Uh oh!

Conversation

shackijj commented May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

shackijj commented May 4, 2020

Uh oh!

shackijj commented May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guybedford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guybedford commented May 4, 2020

Uh oh!

shackijj commented May 6, 2020

Uh oh!

guybedford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guybedford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shackijj commented May 10, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guybedford commented May 18, 2020

Uh oh!

Uh oh!

shackijj commented May 20, 2020

Uh oh!

nodejs-github-bot commented May 20, 2020

Uh oh!

GeoffreyBooth commented May 24, 2020

Uh oh!

GeoffreyBooth commented May 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

shackijj commented May 4, 2020 •

edited

Loading

shackijj commented May 4, 2020 •

edited

Loading