Async hooks and CommonJS loaders #45711
Replies: 9 comments 63 replies
-
I have some concerns over merging the loaders:
|
Beta Was this translation helpful? Give feedback.
-
I'd be -1 such a proposal. I would be ok if the two paths were merged only if a loader was enabled. |
Beta Was this translation helpful? Give feedback.
-
It's worth pointing out that until APMs support loaders, switching off the default CJS module loader would completely break every APM. I don't think we can do this in the short or probably even medium-term. We need to migrate APMs to support loaders first. As far as I'm aware, Datadog is still the only APM with loader support, and only very experimental support, which I suspect is also currently broken on latest Node.js. I'm of the opinion that until loaders is properly stable we should not be considering removing the existing CJS implementation, and even then we will need a full deprecation cycle. |
Beta Was this translation helpful? Give feedback.
-
I ran a quick-and-dirty benchmark to see if a no-op application loaded faster under the CommonJS loader as compared with the ESM loader. They’re equivalent: So simply setting |
Beta Was this translation helpful? Give feedback.
-
I wanted to split this discussion (#45711 (reply in thread), #45711 (reply in thread), #45711 (reply in thread), #45711 (reply in thread), #45711 (reply in thread)) out into its own thread so it doesn’t get lost in the noise:
In other words, when is it acceptable to ship a new feature that only works in CommonJS? Or only in ESM? What if shipping it for both requires breaking changes to the CommonJS module loader or to async hooks? Both CommonJS and ESM are marked as stable, and have been for years (the last version of Node that had ESM as experimental has gone EOL already). When users ask, as they often do, about whether (or when) Node will deprecate CommonJS we always reply that there are no intentions to do so, and that both module systems are fully supported as equals and first-class. In my mind, the implications of that statement are that new features need to support both module systems before the new feature can become stable (if not even earlier in the development process) and that existing features that lack equivalent support in both systems can be said to have a bug that needs fixing, unless there’s something fundamental about the feature that precludes parity. Alternatively, we could change our public position on this. As @joyeecheung wrote above, “I don’t think [the CommonJS loader] being a non-legacy means it needs to get new features. It just needs to be stable and not change much (including not going to get any fancy new features that would break potential edge cases) beyond bug fixes or security fixes. I think sacrificing compatibility (e.g. by making it async in any way) for new features really goes against how we had been advertising it with - an almost-locked, stable, as backwards-compatible as possible component.” Whatever we call this, and both “legacy” and “stable” seem like not-quite-right fits, this is a different promise than both systems being first-class and fully supported. If the spectrum spans from “support both fully” to “CommonJS is locked,” then what doesn’t fit would be new features that only work in CommonJS. That would imply that we’re not supporting ESM fully, when I don’t think there’s any doubt that ESM is stable and not on any path toward legacy or deprecation. If we want to allow new features that are ESM-only, I think that would be fine, providing that we state somewhere that CommonJS users should have no expectation of new features working in CommonJS; but the reverse doesn’t make sense. I’m also fine with a rule that says that new features must work in both unless something fundamentally precludes them from doing so, and the bar for shipping “only works in one system” features should be very high. The practical implications of this question on the discussions above are related to |
Beta Was this translation helpful? Give feedback.
-
This will probably be a very controversial point, but ESM asynchronous nature adds so many challenges, would it make things easier to turn them into CJS? Then we could specify that loaders may export both sync and async versions of their hooks, and use one version or another depending on the context. ESM would use the async version, whereas CJS would use the sync version. Keeping sync and async in sync would be up to loaders' authors. I know a lot of work was already made on the assumption that loaders are implemented in ESM (including off-threading), and I'd myself have to modify the (experimental) loaders my team already ships, so I understand if that doesn't fit the direction. |
Beta Was this translation helpful? Give feedback.
-
Just for context sake the original ESM loader was in C++. I believe it was
ported in 2019 to JS as an effort to be easier to hack on to mixed effects.
…On Thu, Dec 8, 2022, 4:55 PM Geoffrey Booth ***@***.***> wrote:
If we had a better ESM loader implementation I would’ve agreed that it’s
time to prioritize ESM, but unfortunately we don’t. It’s sad that we
already have technical debt in ESM loader even before it actually gets wide
adoption, but I don’t think making the technical debt our bottleneck is the
solution of this problem.
I don’t think it’s productive to blame the ESM loader, as if new features
supporting ESM is blocked by some undefined refactoring that the loader
needs. I’m sure the ESM loader can be improved, but it’s doing a lot of
things—much more than the CommonJS loader—and it’s been stable for years.
There are lots of parts of the codebase that can be improved, and we can’t
tell our users “sorry, but doesn’t work for ESM apps because it was too
much work for us to either get the feature working with the ESM loader we
have or to refactor the ESM loader as needed.” It’s time to prioritize ESM
because if we don’t, any feature that doesn’t work in ESM is incurring
technical debt that we’ll have to pay off when lack of ESM support goes
from annoying to unacceptable.
So if there’s no easy way to get snapshots working with ESM without
refactoring the ESM loader, then whoever is championing snapshots will need
to refactor the ESM loader (or find a way to ship snapshots ESM support
without such a refactoring). Most of the original authors of the ESM loader
aren’t around anymore.
—
Reply to this email directly, view it on GitHub
<#45711 (reply in thread)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABZJI6DCXTJE32UWWEQFJTWMJRNVANCNFSM6AAAAAASRSW5ME>
.
You are receiving this because you are on a team that was mentioned.Message
ID: ***@***.***>
|
Beta Was this translation helpful? Give feedback.
-
If loader hooks are async and as a result also ESM loader must be async. Would this imply that something like this is no longer working if loader hooks are used because requrire is async then? const winston = require("winston")
const logger = winston.createLogger(); Or is this made sync again by moving the whole async loading part into the upcoming loader thread and do an event loop blocking wait on the actual main/worker thread? |
Beta Was this translation helpful? Give feedback.
-
A requested feature for the Loaders API is to have loaders support CommonJS as well as ESM. This would allow libraries like
ts-node
to use a single code path for their customizations, rather than implementing TypeScript transpilation for ESM using loader hooks and again for CommonJS usingrequire.extensions
, for example; and it would allow things that are currently impossible, likerequire
of an HTTPS URL.The technical way to achieve this in the Node codebase is to merge the code in
lib/internal/modules/cjs/
with the far largerlib/internal/modules/esm/
, to have just one loader rather than two thatlib/internal/modules/run_main.js
chooses between. The ESM loader can already load both ESM and CommonJS modules, so the change here is that it would do so forrequire
as well asimport
, and for CommonJS entry points. There would be some tricky problems to solve around promises and synchronous loading; but even if it proves impossible for some reason, there are maintainability reasons for trying to have the current ESM loader at least handle all entry points even if it can’t handle allrequire
calls.In #43408 (comment) and following comments, and #44323 spun off from that discussion, we investigated Node core tests that fail when the ESM loader is used to load a CommonJS entry point. For background, currently the
node
command uses the CommonJS loader unless the entry point is detected as ESM (.mjs
file or.js
under"type": "module"
, or string/STDIN
with--input-type=module
) or if either of the--import
or--loader
flags are passed. Arguably all the Node codebase tests should pass regardless of whether the entry point is handled by the CommonJS or the ESM loader, since the ESM loader can load CommonJS; if any tests fail, that points to possible bugs or brittle tests.I ran the experiment again recently, of setting
run_main.js
line 77 toconst useESMLoader = true
and seeing which tests fail when the ESM loader is always used. In80dcc068fb
there are 160 tests failing, most of which areasync_hooks
. Based on discussion in #44323 and openjs-foundation/summit#340, it seems that most if not all of these failures are expected:createHooks
captures internal async activity, and when the ESM loader is engaged there’s simply more internal async activity going on, and so the hooks are called more often; but the tests are asserting that a specific number of calls should have happened, and therefore the tests fail. Arguably the tests are just too specific, and could be rewritten to ignore hook calls that happen as a result of internal activity rather than async resources created by the tests themselves.Would be acceptable to go ahead and make the ESM loader apply to all entry points (so like
useESMLoader = true
) and update the tests accordingly? As a precursor to thelib/internal/modules/
refactor that would move CommonJS handling into the same flow as ESM and therefore through the ESM loader hooks. Are any users relying on the same expectations as these tests, that there shouldn’t be any internal async activity pending whencreateHooks
is called? If not, or if we’re okay with breaking that expectation, then we can just update the tests. Arguably we should be okay doing that, as even if we considercreateHooks
not quite experimental, no one should be relying on internals being unchanging. What does everyone think, or are there better solutions to consider?@nodejs/tsc @nodejs/async_hooks @nodejs/loaders
Beta Was this translation helpful? Give feedback.
All reactions