nodejs / nodejs/node

Proposal: Have the ESM loader handle all entry points

Đang mở
#50,356 31 bình luận 6 reaction 0 người được giao Xem trên GitHub

Chưa có ai nhận issue này.

esm module never-stale performance
Ngôn ngữ chính
JavaScript
Star
122k
Fork
37.3k
Merge trung bình
4 ngày 2 giờ
Pull request đã merge (30 ngày)
283

Mô tả

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

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Bắt đầu từ đâu

  1. Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
  2. Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
  3. Fork repository và làm thay đổi trên một nhánh.
  4. Mở pull request có tham chiếu số hiệu của issue.

Hướng nghiên cứu

Bắt đầu với lib/internal/modules/run_main.js, đặc biệt là shouldUseESMLoader và nhánh if (useESMLoader), sau đó chạy make test để tái hiện các lỗi. Chọn một nhóm giới hạn từ các test permission, async-hooks hoặc parallel được liệt kê và kiểm tra các assertions hiện tại của nhóm đó. Công việc được xem là hoàn tất khi các test đã chọn đều pass cả trước và khi luôn bật đường dẫn ESM loader, góp phần đưa số lỗi về không.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
javascript, node.js
Lĩnh vực
backend, testing
Loại issue
Tính năng
Độ khó
5/5
Thời gian dự kiến
Hơn một tuần
Mức độ hoạt động
Ít trao đổi
Độ rõ ràng
Khá rõ ràng
Mức phù hợp với người mới
42/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.