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: Unflag --experimental-modules #29866

Closed
wants to merge 6 commits into from

Conversation

@guybedford
Copy link
Contributor

guybedford commented Oct 7, 2019

This PR unflags the --experimental-modules support making modules on-by-default, while remaining backwards-compatible with the current runMain.

This PR should only land after the remaining PRs have been fully considered:

  • Fix 3 remaining task_queue timing issues in this PR (discussed in #29866 (comment))
  • Re-flagging --experimental-json-modules (#29754)
  • Package-relative loading (#29327)
  • Unflagging --experimental-exports (#29867)
  • Bootstrap refactoring (#29937)
  • Resolution on exports fallback to main (#29932)
  • Conditional exports considerations (#29978)
  • Matching ESM extensions order (#29974)

In addition to:

  • Ensuring documentation has been fully reviewed for an unflagged modules release
  • Sign off from the modules group.
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
@guybedford guybedford referenced this pull request Oct 7, 2019
1 of 4 tasks complete
@guybedford guybedford force-pushed the guybedford:unflag-pr branch from 095d7d2 to 5cc5c96 Oct 7, 2019
@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Oct 7, 2019

//cc @nodejs/modules-active-members

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Oct 7, 2019

The three failing tests in this PR are:

  • ./node ./test/addons/async-hooks-promise/test.js the async hook promise injection is not being removed despite being disabled in https://github.com/nodejs/node/pull/29848/files#diff-e6db408e12db906ead6ddfac3de15a6fR314. @jasnell would value your input on this one if you have any suggestions here.
  • ./node ./test/addons/make-callback-recurse/test.js, ./node ./test/node-api/test_make_callback_recurse/test.js both seem due to subtle timing changes with the switch to a promise-based bootstrap.
@guybedford guybedford referenced this pull request Oct 7, 2019
4 of 4 tasks complete
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 9, 2019

Adding WIP label based on PR description. Feel free to remove when appropriate, swap out with blocked label, or whatever. No strong opinions here. Just seems like the right thing to me. (The WIP label helps me in my workflow with the repository, so it's not just cosmetic for me, if that matters. But again, no strong opinions on this particular PR in that regard.)

@guybedford guybedford force-pushed the guybedford:unflag-pr branch 2 times, most recently from f391964 to fe3c3d1 Oct 10, 2019
@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Oct 10, 2019

On @joyeecheung's advice I've gone ahead and changed the way the async bootstrap works. Instead of always applying an async bootstrap, the async bootstrap and all promises associated will only apply lazily when the ESM loader is actually in use. This way loading CommonJS retains a fully sync bootstrap that is completely backwards compatible with no timings changes or task queue differences.

This also removes the dependence of this work on #29848.

All tests are now passing! Further review welcome.

@guybedford guybedford referenced this pull request Oct 10, 2019
6 of 6 tasks complete
@guybedford guybedford force-pushed the guybedford:unflag-pr branch from 84cafc9 to 5a3051a Oct 11, 2019
@guybedford guybedford force-pushed the guybedford:unflag-pr branch from 2eb3b7d to 3958d49 Oct 11, 2019
@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Oct 11, 2019

I've split out the bootstrap changes into a separate PR at #29937, and based this unflagging to that PR for a simpler diff.

@guybedford guybedford force-pushed the guybedford:unflag-pr branch from 3958d49 to 824587c Oct 13, 2019
@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Oct 14, 2019

We likely want #29974 to land as well

@guybedford guybedford force-pushed the guybedford:unflag-pr branch 6 times, most recently from 89b337f to 7e73234 Oct 15, 2019
@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Oct 30, 2019

@nodejs/modules anyone have an issue with renaming --loader to --experimental-loader before we land this? I can open a PR (and 12.x backport), but I'd be concerned with unflagging and not being explicit about this

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

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Nov 9, 2019

Should we keep the flag as a no-op, at least for v13?

Interesting suggestion - I think this could be useful actually. I've gone ahead and pushed a PR to treat --experimental-modules as an alias for the upcoming features we are considering having agreement on for end of Jan - --experimental-conditional-exports and --experimental-self-require, which saves some typing.

If anyone disagrees with this approach though I'd be happy to revert and just remove the flag again.

@targos that's all the feedback, please take another look when you can.

@targos
targos approved these changes Nov 10, 2019
@targos

This comment has been minimized.

Copy link
Member

targos commented Nov 10, 2019

=== release test-process-env-allowed-flags-are-documented ===
Path: parallel/test-process-env-allowed-flags-are-documented
--- stderr ---
assert.js:93
  throw new AssertionError(obj);
  ^
AssertionError [ERR_ASSERTION]: The following options are documented as allowed in NODE_OPTIONS in /home/travis/build/nodejs/node/doc/api/cli.md: --experimental-modules but are not in process.allowedNodeEnvironmentFlags
    at Object.<anonymous> (/home/travis/build/nodejs/node/test/parallel/test-process-env-allowed-flags-are-documented.js:79:8)
    at Module._compile (internal/modules/cjs/loader.js:1176:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1214:10)
    at Module.load (internal/modules/cjs/loader.js:1031:32)
    at Function.Module._load (internal/modules/cjs/loader.js:935:14)
    at Function.Module.runMain (internal/modules/cjs/loader.js:1254:12)
    at internal/main/run_main_module.js:15:11 {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: 1,
  expected: 0,
  operator: 'strictEqual'
}
Command: out/Release/node /home/travis/build/nodejs/node/test/parallel/test-process-env-allowed-flags-are-documented.js
=== release test-esm-exports ===
Path: es-module/test-esm-exports
--- stderr ---
out/Release/node: bad option: --experimental-modules
Command: out/Release/node --experimental-modules /home/travis/build/nodejs/node/test/es-module/test-esm-exports.mjs
===
=== 2 tests failed
===
@guybedford guybedford force-pushed the guybedford:unflag-pr branch from 97bea1f to 814a207 Nov 10, 2019
@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Nov 10, 2019

Those tests are working fine here - seems like there are Travis issues when pushing many commits together I think. I've pushed a rebase, hopefully that should get it working.

@nodejs-github-bot

This comment has been minimized.

@guybedford guybedford referenced this pull request Nov 10, 2019
@nodejs-github-bot

This comment has been minimized.

@jkrems

This comment has been minimized.

Copy link
Contributor

jkrems commented Nov 12, 2019

Are the tests passing locally with a debug build? This smells like an issue where debug asserts are failing (e.g. because of bad handles / unsafe memory access) which isn't always easy to reproduce with the default release build.

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Nov 12, 2019

The failure is just the following, and only on one machine -

not ok 2711 sequential/test-cpu-prof-dir-relative
11:49:49   ---
11:49:49   duration_ms: 0.414
11:49:49   severity: fail
11:49:49   exitcode: 1
11:49:49   stack: |-
11:49:49     Dispatching message { "id": 1, "method": "Profiler.enable" }
11:49:49     Receive CPU profile message, ending = false
11:49:49     Dispatching message { "id": 2, "method": "Profiler.start" }
11:49:49     Receive CPU profile message, ending = false
11:49:49     Dispatching message { "id": 3, "method": "Profiler.setSamplingInterval", "params": { "interval": 50 } }
11:49:49     Receive CPU profile message, ending = false
11:49:49     
11:49:49     assert.js:93
11:49:49       throw new AssertionError(obj);
11:49:49       ^
11:49:49     
11:49:49     AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
11:49:49     
11:49:49     null !== 0
11:49:49     
11:49:49         at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-arm/nodes/centos7-arm64-gcc6/test/sequential/test-cpu-prof-dir-relative.js:39:10)
11:49:49         at Module._compile (internal/modules/cjs/loader.js:1176:30)
11:49:49         at Object.Module._extensions..js (internal/modules/cjs/loader.js:1214:10)
11:49:49         at Module.load (internal/modules/cjs/loader.js:1031:32)
11:49:49         at Function.Module._load (internal/modules/cjs/loader.js:935:14)
11:49:49         at Function.Module.runMain (internal/modules/cjs/loader.js:1254:12)
11:49:49         at internal/main/run_main_module.js:15:11 {
11:49:49       generatedMessage: true,
11:49:49       code: 'ERR_ASSERTION',
11:49:49       actual: null,
11:49:49       expected: 0,
11:49:49       operator: 'strictEqual'
11:49:49     }

Seems very much like a flake to me?

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Nov 12, 2019

The previous error was on a different machine, but also a related type of failure to this -

not ok 2342 parallel/test-worker-prof
14:11:51   ---
14:11:51   duration_ms: 2.343
14:11:51   severity: fail
14:11:51   exitcode: 1
14:11:51   stack: |-
14:11:51     assert.js:93
14:11:51       throw new AssertionError(obj);
14:11:51       ^
14:11:51     
14:11:51     AssertionError [ERR_ASSERTION]: child exited with signal: {
14:11:51       status: null,
14:11:51       signal: 'SIGSEGV',
14:11:51       output: [ null, '', '' ],
14:11:51       pid: 16260,
14:11:51       stdout: '',
14:11:51       stderr: ''
14:11:51     }
14:11:51         at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-arm/nodes/ubuntu1604-arm64/test/parallel/test-worker-prof.js:57:10)
14:11:51         at Module._compile (internal/modules/cjs/loader.js:1176:30)
14:11:51         at Object.Module._extensions..js (internal/modules/cjs/loader.js:1214:10)
14:11:51         at Module.load (internal/modules/cjs/loader.js:1031:32)
14:11:51         at Function.Module._load (internal/modules/cjs/loader.js:935:14)
14:11:51         at Function.Module.runMain (internal/modules/cjs/loader.js:1254:12)
14:11:51         at internal/main/run_main_module.js:15:11 {
14:11:51       generatedMessage: false,
14:11:51       code: 'ERR_ASSERTION',
14:11:51       actual: 'SIGSEGV',
14:11:51       expected: null,
14:11:51       operator: 'strictEqual'
14:11:51     }

are these known to be flakey or does this PR introduce the flakiness then?

@joyeecheung any ideas what might be going on with these?

@nodejs-github-bot

This comment has been minimized.

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Nov 12, 2019

(@jkrems I can confirm the debug build works fine here)

Copy link
Member

MylesBorins left a comment

LGTM

@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Nov 12, 2019

Looks like the last CI was green. Is anything else blocking this landing?

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Nov 12, 2019

Yes, flakes are passing now, so I believe we are good to land.

@jkrems
jkrems approved these changes Nov 12, 2019
Copy link
Contributor

jkrems left a comment

rslgtm

MylesBorins added a commit that referenced this pull request Nov 12, 2019
PR-URL: #29866
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Nov 12, 2019

Landed in 796f3d0

@motss

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.