esm: merge esm and cjs package.json caches

[shackijj]

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]

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

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.

[guybedford]

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

[shackijj]

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

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

[shackijj]

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

[guybedford]

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

[shackijj]

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]

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

[GeoffreyBooth]

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

[GeoffreyBooth]

Landed in 8f10bb2.