nodejs / nodejs/node

Proposal: Have the ESM loader handle all entry points

オープン
#50,356 コメント 31 件 リアクション 6 件 担当者 0 名 GitHub で見る

まだ誰も着手していません。

esm module never-stale performance
主要言語
JavaScript
スター
122k
フォーク
37.3k
平均マージ
4日 2時間
マージ済み PR(30日)
283

説明

In current Node, the ESM loader handles the application entry point when it is an ES module or if any of the following flags are passed: --import, --experimental-default-type, --experimental-detect-module, --experimental-loader. This means that the ESM loader handles CommonJS entry points when certain flags are passed. (The ESM loader supports ES modules, CommonJS modules, WebAssembly modules and JSON files.) A function shouldUseESMLoader in run_main.js determines whether the ESM loader should handle the main entry point.

If we were to remove this function and simply always have the ESM loader handle all entry points, there would be a few gains:

  • There would be only one code path for starting the application entry point, rather than two, simplifying the codebase and making it more maintainable.
  • The simpler the codebase can become, the more likely it will be that we can find performance improvements, such as by moving parts of the flow into C++.

It would also mean the end of monkey-patching the CommonJS loader, which has never been officially supported and I think is ready to be officially retired now that the module customization hooks exist and provide fully equivalent functionality, including for customizing require calls. They are Release Candidate and I expect them to become stable in the next few months. This potential change of having the ESM loader handle all entries would happen after that, at the next major version of Node. It shouldn’t be a semver-major change, but monkey-patching is common enough that I’d expect some breakage and therefore we should treat it as semver-major.

If you take the run_main.js if (useESMLoader) { line and change it to if (true) { and run make test, as of today there are 154 failing tests. For the most part, these are tests that just need updating because the assertions in the tests are more specific than they need to be for what they’re validating. They test things like the number of promises created since Node started up, which varies when the ESM loader is engaged because the loader itself creates a few; or they need --allow-fs to include allowing reading some package.json files that the ESM loader loads that the CommonJS loader didn’t; or they are validating too precise of a stack trace, that changes when the ESM loader is used; and so on. The largest group of failing tests is for async hooks, and I’ve discussed this with @Qard (see https://github.com/nodejs/node/pull/44323#issuecomment-1775434155) and we think that those tests should simply be updated, once the major refactor of AsyncLocalStorage lands.

In the meantime, I think we should start making smaller PRs to start updating batches of tests that fail when shouldUseESMLoader is always true, so that these tests pass both today and in “always using the ESM loader” mode. Once we can get that 154 number down to zero, then we can do the cutover, ideally before Node 22 to be released as part of 22.0.0. This issue can serve as the tracking issue for the effort.

These are the tests (or APIs) that need updating:
  • ./node --experimental-permission --allow-fs-read=* --allow-child-process test/parallel/test-permission-fs-wildcard.js
  • ./node --experimental-permission --allow-fs-read=* --allow-fs-write=* --allow-child-process test/parallel/test-permission-fs-read.js
  • ./node --experimental-permission --allow-fs-read=* --allow-fs-write=* --allow-child-process test/parallel/test-permission-fs-symlink-target-write.js
  • ./node --experimental-permission --allow-fs-read=* --allow-fs-write=* --allow-child-process test/parallel/test-permission-fs-symlink.js
  • ./node --experimental-permission --allow-fs-read=* --allow-fs-write=* --allow-child-process test/parallel/test-permission-fs-traversal-path.js
  • ./node --experimental-permission --allow-fs-read=* test/parallel/test-permission-experimental.js
  • ./node --expose-gc test/async-hooks/test-pipewrap.js
  • ./node --expose-gc test/async-hooks/test-querywrap.js
  • ./node --expose-gc test/parallel/test-heapdump-async-hooks-init-promise.js
  • ./node --expose-internals test/async-hooks/test-emit-after-on-destroyed.js
  • ./node --expose-internals test/async-hooks/test-emit-before-on-destroyed.js
  • ./node --expose-internals test/async-hooks/test-emit-init.js
  • ./node --expose-internals test/async-hooks/test-http-agent-handle-reuse-parallel.js
  • ./node --expose-internals test/async-hooks/test-http-agent-handle-reuse-serial.js
  • ./node --expose-internals test/async-hooks/test-improper-order.js
  • ./node --expose-internals test/async-hooks/test-zlib.zlib-binding.deflate.js
  • ./node --expose-internals test/message/internal_assert_fail.js
  • ./node --expose-internals test/message/internal_assert.js
  • ./node --expose-internals test/parallel/test-bootstrap-modules.js
  • ./node --expose-internals test/parallel/test-cluster-dgram-bind-fd.js
  • ./node --expose-internals test/parallel/test-util-promisify.js
  • ./node --inspect=0 test/known_issues/test-inspector-cluster-port-clash.js
  • ./node --no-warnings --expose-internals test/parallel/test-whatwg-webstreams-adapters-to-writablestream.js
  • ./node --pending-deprecation test/parallel/test-module-parent-deprecation.js
  • ./node --test-udp-no-try-send test/async-hooks/test-udpsendwrap.js
  • ./node test/addons/callback-scope/test-async-hooks.js
  • ./node test/addons/make-callback-recurse/test.js
  • ./node test/addons/report-api/test.js
  • ./node test/async-hooks/test-async-await.js
  • ./node test/async-hooks/test-crypto-pbkdf2.js
  • ./node test/async-hooks/test-crypto-randomBytes.js
  • ./node test/async-hooks/test-disable-in-init.js
  • ./node test/async-hooks/test-embedder.api.async-resource-no-type.js
  • ./node test/async-hooks/test-embedder.api.async-resource.js
  • ./node test/async-hooks/test-enable-disable.js
  • ./node test/async-hooks/test-enable-in-init.js
  • ./node test/async-hooks/test-filehandle-no-reuse.js
  • ./node test/async-hooks/test-fseventwrap.js
  • ./node test/async-hooks/test-fsreqcallback-access.js
  • ./node test/async-hooks/test-fsreqcallback-readFile.js
  • ./node test/async-hooks/test-getaddrinforeqwrap.js
  • ./node test/async-hooks/test-getnameinforeqwrap.js
  • ./node test/async-hooks/test-graph.fsreq-readFile.js
  • ./node test/async-hooks/test-graph.http.js
  • ./node test/async-hooks/test-graph.intervals.js
  • ./node test/async-hooks/test-graph.pipe.js
  • ./node test/async-hooks/test-graph.pipeconnect.js
  • ./node test/async-hooks/test-graph.shutdown.js
  • ./node test/async-hooks/test-graph.signal.js
  • ./node test/async-hooks/test-graph.statwatcher.js
  • ./node test/async-hooks/test-graph.tcp.js
  • ./node test/async-hooks/test-graph.timeouts.js
  • ./node test/async-hooks/test-graph.tls-write-12.js
  • ./node test/async-hooks/test-graph.tls-write.js
  • ./node test/async-hooks/test-httpparser.request.js
  • ./node test/async-hooks/test-httpparser.response.js
  • ./node test/async-hooks/test-immediate.js
  • ./node test/async-hooks/test-nexttick-default-trigger.js
  • ./node test/async-hooks/test-pipeconnectwrap.js
  • ./node test/async-hooks/test-promise.chain-promise-before-init-hooks.js
  • ./node test/async-hooks/test-promise.js
  • ./node test/async-hooks/test-promise.promise-before-init-hooks.js
  • ./node test/async-hooks/test-queue-microtask.js
  • ./node test/async-hooks/test-shutdownwrap.js
  • ./node test/async-hooks/test-signalwrap.js
  • ./node test/async-hooks/test-statwatcher.js
  • ./node test/async-hooks/test-tcpwrap.js
  • ./node test/async-hooks/test-timers.setInterval.js
  • ./node test/async-hooks/test-timers.setTimeout.js
  • ./node test/async-hooks/test-tlswrap.js
  • ./node test/async-hooks/test-ttywrap.readstream.js
  • ./node test/async-hooks/test-udpwrap.js
  • ./node test/async-hooks/test-unhandled-exception-valid-ids.js
  • ./node test/async-hooks/test-unhandled-rejection-context.js
  • ./node test/async-hooks/test-writewrap.js
  • ./node test/message/assert_throws_stack.js
  • ./node test/message/util_inspect_error.js
  • ./node test/message/util-inspect-error-cause.js
  • ./node test/node-api/test_callback_scope/test-async-hooks.js
  • ./node test/node-api/test_make_callback_recurse/test.js
  • ./node test/node-api/test_policy/test_policy.js
  • ./node test/parallel/test-async-hooks-correctly-switch-promise-hook.js
  • ./node test/parallel/test-async-hooks-disable-during-promise.js
  • ./node test/parallel/test-async-hooks-enable-recursive.js
  • ./node test/parallel/test-async-hooks-promise-triggerid.js
  • ./node test/parallel/test-async-hooks-promise.js
  • ./node test/parallel/test-async-hooks-top-level-clearimmediate.js
  • ./node test/parallel/test-async-local-storage-exit-does-not-leak.js
  • ./node test/parallel/test-async-wrap-promise-after-enabled.js
  • ./node test/parallel/test-child-process-fork-closed-channel-segfault.js
  • ./node test/parallel/test-cli-permission-deny-fs.js
  • ./node test/parallel/test-cluster-advanced-serialization.js
  • ./node test/parallel/test-cluster-dgram-2.js
  • ./node test/parallel/test-cluster-fork-windowsHide.js
  • ./node test/parallel/test-cluster-send-deadlock.js
  • ./node test/parallel/test-cluster-send-socket-to-worker-http-server.js
  • ./node test/parallel/test-cluster-worker-disconnect-on-error.js
  • ./node test/parallel/test-cluster-worker-events.js
  • ./node test/parallel/test-cluster-worker-forced-exit.js
  • ./node test/parallel/test-cluster-worker-init.js
  • ./node test/parallel/test-cluster-worker-no-exit.js
  • ./node test/parallel/test-debugger-break.js
  • ./node test/parallel/test-debugger-breakpoint-exists.js
  • ./node test/parallel/test-debugger-clear-breakpoints.js
  • ./node test/parallel/test-debugger-exceptions.js
  • ./node test/parallel/test-debugger-exec-scope.mjs
  • ./node test/parallel/test-debugger-extract-function-name.mjs
  • ./node test/parallel/test-debugger-heap-profiler.js
  • ./node test/parallel/test-debugger-help.mjs
  • ./node test/parallel/test-debugger-list.js
  • ./node test/parallel/test-debugger-object-type-remote-object.js
  • ./node test/parallel/test-debugger-profile-command.js
  • ./node test/parallel/test-debugger-profile.js
  • ./node test/parallel/test-debugger-random-port-with-inspect-port.js
  • ./node test/parallel/test-debugger-random-port.js
  • ./node test/parallel/test-debugger-run-after-quit-restart.js
  • ./node test/parallel/test-debugger-sb-before-load.js
  • ./node test/parallel/test-debugger-scripts.js
  • ./node test/parallel/test-debugger-set-context-line-number.mjs
  • ./node test/parallel/test-debugger-use-strict.js
  • ./node test/parallel/test-debugger-watch-validation.js
  • ./node test/parallel/test-debugger-watchers.mjs
  • ./node test/parallel/test-diagnostics-channel-process.js
  • ./node test/parallel/test-error-reporting.js
  • ./node test/parallel/test-inspector-debug-brk-flag.js
  • ./node test/parallel/test-inspector-exception.js
  • ./node test/parallel/test-inspector.js
  • ./node test/parallel/test-module-main-preserve-symlinks-fail.js
  • ./node test/parallel/test-node-output-console.mjs
  • ./node test/parallel/test-node-output-errors.mjs
  • ./node test/parallel/test-node-output-sourcemaps.mjs
  • ./node test/parallel/test-node-output-vm.mjs
  • ./node test/parallel/test-performance-nodetiming.js
  • ./node test/parallel/test-process-external-stdio-close-spawn.js
  • ./node test/parallel/test-process-external-stdio-close.js
  • ./node test/parallel/test-process-uncaught-exception-monitor.js
  • ./node test/parallel/test-promise-hook-create-hook.js
  • ./node test/parallel/test-promise-hook-exceptions.js
  • ./node test/parallel/test-promise-hook-on-after.js
  • ./node test/parallel/test-promise-hook-on-resolve.js
  • ./node test/parallel/test-runner-output.mjs
  • ./node test/parallel/test-sync-io-option.js
  • ./node test/parallel/test-v8-coverage.js
  • ./node test/parallel/test-worker-debug.js
  • ./node test/parallel/test-worker-load-file-with-extension-other-than-js.js
  • ./node test/parallel/test-worker-message-port-inspect-during-init-hook.js
  • ./node test/sequential/test-debugger-custom-port.js
  • ./node test/sequential/test-debugger-launch.mjs
  • ./node test/sequential/test-module-loading.js
  • ./node test/sequential/test-perf-hooks.js
  • ./node test/sequential/test-watch-mode.mjs
  • python test/pseudo-tty/../../tools/pseudo-tty.py ./node test/pseudo-tty/console_colors.js
  • python test/pseudo-tty/../../tools/pseudo-tty.py ./node test/pseudo-tty/test-fatal-error.js
  • python test/pseudo-tty/../../tools/pseudo-tty.py ./node test/pseudo-tty/test-trace-sigint.js

@nodejs/loaders @nodejs/tsc @nodejs/performance @nodejs/async_hooks

コントリビューションガイド

コントリビューションガイドを開く

はじめの一歩

  1. issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
  2. 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
  3. リポジトリをフォークし、ブランチを切って変更します。
  4. issue 番号を参照したプルリクエストを送ります。

調査の方向性

lib/internal/modules/run_main.js から始め、特に shouldUseESMLoader と if (useESMLoader) ブランチを確認してから、make test を実行して失敗を再現します。リストにある permission、async-hooks、または parallel テストから範囲を限定したグループを選び、現在の assertions を調べます。選択したテストが、通常の場合と ESM loader パスを常に有効にした場合の両方で成功し、失敗数ゼロに向けて貢献できれば完了です。

索引モデルが issue の本文から書いたものです。

評価

技術スタック
javascript, node.js
領域
backend, testing
issue の種類
機能追加
難易度
5/5
見積もり時間
1週間以上
活発さ
静か
明瞭さ
おおむね明確
初心者へのやさしさ
42/100

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。