Repository navigation
Proposal: Have the ESM loader handle all entry points #50356
Description
Activity
I think that we need stable loaders first and at least a full deprecation cycle, which would mean deprecation of customizing require in v22 and removal in v23.
Reacted by Stephen Belanger and Alexander PraetoriusWhat's the point of calling CJS "stable" for over a decade if breaking it is ever on the table?
Reacted by Stephen Belanger, Tobias Nießen and Alexander Praetoriuswhich would mean deprecation of customizing require in v22 and removal in v23.
Updating all these tests might not be ready in time for 22 regardless, but I think we should still get started. We don't need to debate just yet whether this should happen in 22 or 23.
Let's add the deprecation warning now. The hooks are hopefully stable, I'm just waiting for any bug reports to trickle in before opening a PR to make it official. And to be clear, the deprecation isn't around customizing require; that is supported via the hooks. The deprecation is around monkey patching, which isn't part of our API today and never has been.
And to be even clearer: this proposal doesn't involve removing the CommonJS loader. It just means that monkey patching wouldn't affect the entry point, since that's effectively imported when loaded by the ESM loader. I think monkey patching would still work for any modules required by a CommonJS entry point.
If it's something that's worked for a decade in a module system that's been called "stable" - meaning, everything that's possible to do with it is part of the API - then it's something that shouldn't be broken.
Specifically, monkey patching require.extensions needs to work forever.
Reacted by Stephen Belanger, Benjamin Gruenbaum, Klaus L. Hougesen and Reece DunhamReacted by Shrujal Shah, Sindre Sorhus, Jacob Hummer and Frank LemanschikReacted by Fabian MeyerFor now the customization hooks do not appear to be equivalent for the customization of
require, it needs a runtime flag whereas monkey-patching of require does not. For example this means popular modules like https://www.npmjs.com/package/v8-compile-cache would be broken.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
This look like a problem of the ESM loader itself, instead of a problem of the test?
As for the gains mentioned in the OP:
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++.I am not fully convinced by this. The CommonJS loader as it stands is currently much simpler than the ESM loader. It's also easier to optimize and snapshot, whereas the ESM loader is currently much more complex and involves a lot of unnecessary operation during initialization. The ESM loader also relies on TDZs and circular dependencies to work, which is a footgun. I think those should be refactored out and cleaned up before we can consider making it the only entry point, otherwise this only results in breakage in the ecosystem + performance regressions with only "potential performance gains", but not effective ones. The proposal now seems to be mostly about an internal refactoring, I think we should do it the other way around - improve the current ESM loader implementation so that there are actually performance gains by using it by default, put this behind a flag, and use the flag to test it and prove that it improves performance & can minimize breakages in the ecosystem, then flip the flag and remove the CommonJS loader when we have proof (possibly leaving the CommonJS loader around for a few cycles so that people can revert back with a flag when problem arises).
Reacted by Stephen Belanger, Vladimir Grenaderov and Jordan HarbandCustomization hooks don't need a runtime flag anymore. We even recommend to use
module.registerinstead.ESM hooks require an additional command line option
--importto get them installed early enough.
Monkey patching CJS loader works also if done early enough in the main application file. Quite some tools relying on monkey patching CJS loader have something like "require me early" in their docs.Reacted by Stephen BelangerESM hooks require an additional command line option --import to get them installed early enough.
Even when the entry point is CommonJS? I would expect this to work:
const { register } = require('module'); register(...) // customized const something = require('something')Customization hooks don't need a runtime flag anymore. We even recommend to use
module.registerinstead.node --import register-hook-a.js --import register-hook-b.jsis not the same asnode --loader loader-a.js --loader-b.jsas an example - imported modules loaded in parallel so there is no chaining as we have with --loader or synchronous --require.So it's hard say that 'we are stable, all use cases covered by hooks API & ESM loader'.
ESM hooks require an additional command line option --import to get them installed early enough.
Even when the entry point is CommonJS? I would expect this to work:
const { register } = require('module'); register(...) // customized const something = require('something')This only works if that CJS module (the one calling
require) was loaded by the ESM loader – atm the ESM loader will delegate the loading of CJS files to the CJS loader; however you can opt-in with e.g.--experimental-default-type=moduleto have the ESM loader handle everything by default, in which case the above snippet should work as expected.Customization hooks don't need a runtime flag anymore. We even recommend to use
module.registerinstead.node --import register-hook-a.js --import register-hook-b.jsis not the same asnode --loader loader-a.js --loader-b.jsas an example - imported modules loaded in parallel so there is no chaining as we have with --loader or synchronous --require.So it's hard say that 'we are stable, all use cases covered by hooks API & ESM loader'.
You can register
loaderscustomization hooks using--require.8 remaining items
Sorry for such offtopic, it’s a good example shows that current ESM APIs are… unfinished?
I disagree that they’re unfinished, but why don’t you open a separate issue for this? I do agree that it’s off-topic for this one 😄
Reacted by Vladimir GrenaderovThanks, you’re right, but there’s still a lot of packages created during that time period, with that expectation. The breakage will be quite significant, and “use loaders instead” isn’t a viable option for most of these use cases.
I agree on the brekage impact. However it's generically possible to track the deprecation and act on it only after we have minimized the disruption.
I'm generically done with supporting unmaintained modules forever.
What use cases are not covered by the new register API?
I would like to ensure that a module that has patched
require.extensionswill continue to work forever. I have no opinion or attachment on/to the internal implementation; having only the one loader path for everything seems ideal, modulo #50356 (comment).I don't think that "but we'd have only one implementation!" is compelling enough to warrant literally any breakage of any kind, but if it can be done in a nonbreaking way, the benefits are obvious.
What use cases are not covered by the new register API?
I don't think the hooks allow you to customize how the modules are compiled? That's why we have specifically have this branch in the CommonJS loader
node/lib/internal/modules/cjs/loader.js
Line 1264 in 4f5db8b
and there are some pretty popular modules like https://www.npmjs.com/package/v8-compile-cache that depend on patching
Module.prototypemethods. Judging from the number of downloads of that package, I think we need to be careful not to create performance regressions in the module loading. I am pretty sure this package is not the only one that relies on the patchability of the CommonJS loader, so the flip should be done after investigation into the breakage + providing enough equivalents. So far what I've seen are only vague evaluations of the breakage and migration paths that aren't very concrete.Reacted by Benjamin GruenbaumAlso some questions about the register API:
- It's run on a different thread which comes with a performance impact that simple monkey-patching doesn't have and that should be dealt with. Also running it on a different thread can already result in behavior parity.
- The hooks are async, how are they supposed to provide equivalent behavior of the original monkey-patchability of the CommonJS loader which is synchronous?
2. The hooks are async, how are they supposed to provide equivalent behavior of the original monkey-patchability of the CommonJS loader which is synchronous?
That's the point of running them on a different thread (that and encapsulation), it lets us run async code in a way that appears sync to the main thread.
I don’t think the hooks allow you to customize how the modules are compiled?
So part of the plan for the loaders/customization team is to add customization hooks for other subsystems, like
import { register } from 'node:fs'and fromnode:repletc. What you’re describing sounds like a potentialcompilehook that can be used via a potentialimport { register } from 'node:vm'. Creating such a hook would also allowv8-compile-cacheto support ES modules.Also some questions about the register API:
Let’s discuss this in a new thread: #50392
Whether or not there are use case gaps with the customization hooks doesn’t really matter right now; we’re not on the verge just yet of making the end-goal change, of having the ESM loader handle all entry points. Unless we think that we should never make such a change, we can defer that evaluation until we’re ready to open a PR to do it. In the meantime, I’d like for us to get started updating the tests I mentioned in the top post, so that we’re eventually in a position to make the change.
FWIW #54592 might resolve some of the issues (not even close to all tho)
I don't think the two are related, #54592 only removes a bogus assertion when
--importis used together with an ESM entry point under--expreimental-require-module. In that case the entry point is still handled by the CJS loader as before, just no longer with a bogus assertion.I also believe the proposal here has not really been relevant now that the CJS loader can load ESM. Technically, there has always been just one loader that is what's also exposed as
module.Moduleand it has always been handling the entry points. The ESM pieces used to be just helpers that were referenced by this loader when it tries to load ESM - and all the headaches only came from it not trying hard enough and tended to give up with an error (e.g. it used to only invoke the ESM helpers when it encounteredimport(), but otherwise gave up whenrequire()led to an ESM, or the entry point appeared to be an ESM). But now that this part tries harder to find ways to invoke the ESM loading helpers, there is no point to make the other loader - which is only an abstracted away collection of the ESM loading helpers - the only thing that can load modules. We could just leave them as helpers and reuse the shared logic more, and there won't be unnecessary breakages if we do the refactoring this way.I don't think the two are related, #54592 only removes a bogus assertion when --import is used together with an ESM entry point under --expreimental-require-module. In that case the entry point is still handled by the CJS loader as before, just no longer with a bogus assertion.
When testing, I experiencing some test failures be resolved after that PR, but maybe something else changed locally on my setup.
This issue has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.
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 functionshouldUseESMLoaderinrun_main.jsdetermines 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:
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
requirecalls. 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.jsif (useESMLoader) {line and change it toif (true) {and runmake 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-fsto include allowing reading somepackage.jsonfiles 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 #44323 (comment)) and we think that those tests should simply be updated, once the major refactor ofAsyncLocalStoragelands.In the meantime, I think we should start making smaller PRs to start updating batches of tests that fail when
shouldUseESMLoaderis 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.mjspython test/pseudo-tty/../../tools/pseudo-tty.py ./node test/pseudo-tty/console_colors.jspython test/pseudo-tty/../../tools/pseudo-tty.py ./node test/pseudo-tty/test-fatal-error.jspython test/pseudo-tty/../../tools/pseudo-tty.py ./node test/pseudo-tty/test-trace-sigint.js@nodejs/loaders @nodejs/tsc @nodejs/performance @nodejs/async_hooks