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.

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

esm: merge esm and cjs package.json caches #33229

Closed

Conversation

shackijj
Copy link
Contributor

@shackijj shackijj commented May 4, 2020

Relates to: #30674

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

@nodejs-github-bot nodejs-github-bot added the esm Issues and PRs related to the ECMAScript Modules implementation. label May 4, 2020
@shackijj
Copy link
Contributor Author

shackijj commented May 4, 2020

@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 shackijj force-pushed the sharing-package-json-cache-#30674 branch from adcfd60 to c14e769 Compare May 4, 2020 12:55
@shackijj
Copy link
Contributor Author

shackijj commented May 4, 2020

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?

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/resolve.js Show resolved Hide resolved
@guybedford
Copy link
Contributor

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

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

shackijj commented May 6, 2020

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

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

@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 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
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

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

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

add test for ERR_INVALID_PACKAGE_CONFIG error

Refs: nodejs#30674
@shackijj shackijj force-pushed the sharing-package-json-cache-#30674 branch from 4f19db7 to 0227b07 Compare May 10, 2020 09:07
@shackijj
Copy link
Contributor Author

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

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

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

src/node_file.cc Outdated Show resolved Hide resolved
@shackijj shackijj force-pushed the sharing-package-json-cache-#30674 branch from d760659 to 0d947ec Compare May 20, 2020 08:04
@shackijj
Copy link
Contributor Author

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
Copy link
Collaborator

@GeoffreyBooth
Copy link
Member

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

@GeoffreyBooth 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
Refs: #30674

PR-URL: #33229
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
@GeoffreyBooth
Copy link
Member

Landed in 8f10bb2.

@GeoffreyBooth 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
@addaleax addaleax mentioned this pull request May 25, 2020
4 tasks
codebytere pushed a commit that referenced this pull request Jun 18, 2020
Refs: #30674

PR-URL: #33229
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
@codebytere codebytere mentioned this pull request Jun 28, 2020
codebytere pushed a commit that referenced this pull request Jun 30, 2020
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
Refs: #30674

PR-URL: #33229
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
@codebytere codebytere mentioned this pull request Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants