module: proxy require('esm') to import('esm')

[MylesBorins]

This PR changes the behavior when someone attempts to require and ESM module.

Rather than throwing, which we currently do, the module that was attempted to be required will be proxied to dynamic import and require will return the promise to resolve the module (the same behavior as dynamic import).

This works for both packages and standalone files.

This is an alternative to #30891

One thing worth mentioning is that it would appear that the majority of folks that are wrapping this error right now are doing so for the exact behavior that this PR implements

https://github.com/search?q=err.code+%3D%3D+ERR_REQUIRE_ESM&type=code

There are currently no docs, assuming we are open to landing this PR I'll update documentation

[aduh95]

@nodejs/modules

[guybedford]

One thing we could possibly do here to improve the error message currently would be to use a parser to determine if the target file looks like CommonJS or ESM, then alter the message based on those two cases.

• If the file is CommonJS, then note that "type": "module" is in play so they should rename the file to ".cjs" or remove "type": "module"
• If the file is ESM, then the message could simplify to just "use dynamic import(), which is supported in both CJS and ESM"

Would something like that help to remove this confusion?

[ljharb]

Wouldn't this mean that by writing ESM, i'm automatically forced to support an API that's both async behind a Promise, and sync-ish via import? How would I prevent someone from requiring an ESM file, in favor of the CJS file I also provided (and mapped to via "exports")?

[bmeck]

I think this needs to be converted to a URL for windows paths.

[weswigham]

It also makes sense to expose the inners of getModuleJob directly and use that rather than import (namely all the bits after the specifier is resolved, like in the other PR) - the cjs loader has already resolved the specifier at this point so there's no reason to have the esm loader double resolve it. (That's just more unneeded disk hits, and potential format confusion bugs if the two loaders disagree on the format)

[bcomnes]

This just confuses the problem further imo. The vast majority of situations where CJS will try to require ESM is existing code tracking dependency updates on packages that have decided to go from CJS to pure ESM (perhaps unknowingly or via dependabot or whatever).

This change (afaict) simply disguises require as dynamic import(), changing the import from a sync to async context, and thus requiring at a minimum, rewriting the code below in a promise callback, in addition to changing any export semantics downstream (a non-trivial rewrite). At this point, rewriting require to import().then() is really the least of your problem and throwing is the best you can do instead of passing off a promise where something else is expected. People wrapping the error are accommodating dynamic import scenarios, or specifically telling people 'this tool only works with cjs right now' (in which case this would then break those peoples code).

#30891 actually attempts to solve a sync resolution of an esm module inside of a require statement which would be a much better solution if we can agree on a way to make it work.

Finally, replacing an error with behavior that is already served fine with import will almost certainly block the more ideal solution everyone wants: a sync resolution of a require('mjs'). We should really reserve the behavior path for anyone who can find a will and way for that to happen. I hope @weswigham can comment.

[VoltrexKeyva]

Suggested change

	process.emitWarning('Attempting to require and ESModule.' +
	process.emitWarning('Attempting to require and ESModule. ' +

Tests

[VoltrexKeyva]

Suggested change

	process.emitWarning('Attempting to require and ESModule.' +
	process.emitWarning('Attempting to require and ESModule. ' +

Tests

[weswigham]

This is a dangerous PR to even propose - I don't wanna start seeing await require in the wild (and accompanying asyncification) "just in case". ☹️ Plus, this would make a require of an esm module indistinguishable from a require of a cjs module which module.export ='s a Promise, which, unlike the esm module, has some sync executing upfront parts. (And that's not even to mention the trouble you get into when you end up awaiting a cjs module with a then export!)

The problem was never require not resolving esm modules - the problem is require not being able to resolve esm modules synchronously, imo.

[chrisdickinson]

Hi! I asked for this change on twitter. I'm interested in addressing the following user flow:

1. Sam is unfamiliar with node and JavaScript. They wish to use another module from their JS file, foo.js, already containing other require calls.
2. They type require("./bar").
3. Node errors and tells them to use import. Sam scans the error and don't read it fully, because "dynamic import()" doesn't make sense to them.
4. They change the require to import * from './bar.js'.
5. Node errors and tells them that import is not valid in a CJS module. Sam is confused and changes the filename to .mjs.
6. Node errors and tells them that require is not available in ESM. Sam is frustrated.

Now, notably, I find "make Sam read the error message and grok dynamic import()" unacceptable as an answer, personally. So that's a no-go for me, at least. I'm basing this on a similar story posted on Twitter.

As for sync resolution of modules, Node needs to make a call one way or another. Like, it's within the realm of possibility that such a thing could be made to work, but is that desirable? I don't know. It seems like a pretty big divergence in behavior from how modules work in other environments, though it does bring it closer to ES module syntax as transpiled to CJS. (At that point, why not transpile optimistically?) In order to make a call on this PR, we need to be able to make a call on that PR.

Speaking to it being dangerous, well, I disagree. It looks like folks are already seeking out this behavior, according to that code search.

[weswigham]

Now, notably, I find "make Sam read the error message and grok dynamic import()" unacceptable as an answer, personally. So that's a no-go for me, at least. I'm basing this on a similar story posted on Twitter.

There's no difference in usage between requiring await require and await import, except a host of edge cases that can go wrong because of legacy behavior of require (like "thenable" cjs modules or cjs modules export assigned to a promise). You're asking for the complexity of the async loader to be hidden, because if we're honest most users would rather not be confronted with it, but this change doesn't do that - it just trades in one set of footguns (with very well documented edges) for another (with fuzzier, harder to explain edges). And moreover, you still have to deal with the complexity of asyncificying your code. (Which, IMO, is going to be a much bigger issue than simply "grokking dynamic import" - if you're going to have trouble with dynamic import, you're probably going to have trouble with a require that sometimes behaves like dynamic import!)

IMO, for now, @guybedford is probably right - there's probably room to improve the error message in CJS to directly include a copy-pasteable dynamic import statement that can be used instead of require. If this were in TS, we'd probably suggest adding a quick fix that replaces the require with a dynamic import and makes the containing function async.

[chrisdickinson]

There's no difference in usage between requiring await require and await import, except a host of edge cases that can go wrong because of legacy behavior of require (like "thenable" cjs modules or cjs modules export assigned to a promise). You're asking for the complexity of the async loader to be hidden, because if we're honest most users would rather not be confronted with it, but this change doesn't do that - it just trades in one set of footguns (with very well documented edges) for another (with fuzzier, harder to explain edges). And moreover, you still have to deal with the complexity of asyncificying your code. (Which, IMO, is going to be a much bigger issue than simply "grokking dynamic import" - if you're going to have trouble with dynamic import, you're probably going to have trouble with a require that sometimes behaves like dynamic import!)

I don't know if I agree with this take on the goals of this. I'm not asking for the complexity of the loader to be hidden, I'm saying that Node has enough context at this point to know how to appropriately load the target module, and the current messaging is sending users down a path that leads to frustration. Whether users use import() directly or use require('esm'), users would get a promise, so that's a neutral value. How many popular then-able CJS modules are there in use today? Or CJS modules that export a promise that shouldn't be awaited? Is that a footgun that people are currently running into, or are you worried that it opens the door to the potential for that footgun?

Better error messaging would be an improvement, but in this case, the suggestion in the error seems like it'd always be adopted by the user. In which case, why not just make the thing the user expected to work... work? (Leaving aside the fact that that approach closes the door to synchronously requiring ESM modules for now.)

[weswigham]

I'm basing this on a similar story posted on Twitter.

Moreover, at least in that story, if this behavior was in already, you'd have seen the same story posted, since none of the lines given would have worked still, except the error in the require cases would have been Property 'x' is undefined instead of the at least slightly more leading You can't use require on a mjs file error. I'm not sure this actually helps - I still think making the error more helpful is the way to go.

[bcomnes]

I'm basing this on a similar story posted on Twitter.

Moreover, at least in that story, if this behavior was in already, you'd have seen the same story posted, since none of the lines given would have worked still, except the error in the require cases would have been Property 'x' is undefined instead of the at least slightly more leading You can't use require on a mjs file error. I'm not sure this actually helps - I still think making the error more helpful is the way to go.

A bunch of those esm error handling examples don't actually handle the error and simply recontextualize it to some form of "cjs only sorry". This change would break that error handling in exactly the way you describe, in addition to preventing a real require('foo.mjs') solution in the future. Is that the intention here as well? I refuse to accept giving that possibility up (because it would solve the whole mess), even if it never materializes 🥲 Maybe someone will figure it out.

For the case of a beginner learning about the world of node module types, improving the error message would be the only option here that doesn't inadvertently introduce many more issues.

[giltayar]

@MylesBorins this is a breaking change, and will break the ESM handling in Mocha v7 & v8, because we rely on the exception ERR_REQUIRE_ESM being thrown to try to import instead (https://github.com/mochajs/mocha/blob/1c4e623c020c97269922dc9e23a951f63ad8f7b8/lib/esm-utils.js#L43).

BTW, this won't break Mocha v9 and onward, because in v9 we first import, and if that fails, we require, so that's OK, but anyone using ESM with v7 and v8 will break.

[GeoffreyBooth]

Here’s how I would imagine this playing out if I were the user:

1. I try to do const { foo } = require('esm'). It doesn’t error, but my code doesn’t work as expected. Why is foo undefined?
2. After some debugging I realize that the value of require('esm') is a Promise. I rewrite this to const { foo } = await require('esm').
3. I think that await require looks wrong, since generally require is synchronous, and so I change this to const { foo } = await import('esm') instead. And now we’re back to code that works today.

[targos]

Except it still doesn't work, because there is no top-level await in CommonJS

[MylesBorins]

Does anyone want to explicitly block this change? If people would rather see this behind a flag (at least to start) that could be an option too.

Please let me know as I don't want to kick off additional work on this if it isn't going to land.

I don't personally see there being a future where we can actually support synchronous require of ESM and I don't think it is benefiting our users continuing to throw here to carve out the future design space at the cost of today's UX

[targos]

I won't block, but I really don't see how this benefits our users.

[devsnek]

i am too tired to block but i am not a fan

[targos]

The common scenario that I imagine with this change is:

• User installs module X, which in its latest version only supports ESM (but User doesn't know)
• User does what they usually do with the module, for example:

const { someFunction } = require('X')
module.exports = function() {
  someFunction(someArgument)
}

• When User executes their code, they see an error TypeError: someFunction is not a function
• They don't understand, because last time they used this module, someFunction was a function
• User opens the Node.js REPL and does require('X').
• It prints Promise { <pending> }
• Then what?

[weswigham]

I don't personally see there being a future where we can actually support synchronous require of ESM

I get that the many comments in the other thread are hard to follow, but the TL;DR at the end of the discussion in it is that we can avoid the event loop chicanery once loaders are moved off-thread (which is still planned to be done...? @bmeck ) by using some Atomics.wait calls, since the only actual async part of es module loading is the loader API. Now, you could avoid waiting for that future by duplicating the implementation that exists today with a sync version, and throwing if loading needs a promise (because a custom experimental loader is in use under the current API) or execution returns a promise (indicating TLA), and some people might even prefer that duplication, since it would allow the theoretical parallel loading of modules in non-require scenarios, but that's a bit complicated, since you need to duplicate most of ModuleJob to have not-async-flagged methods. (Notable: the current implementation of ModuleJob might not be to spec as-is, since the current implementation allows an observable event loop iteration between every module in a non-TLA module graph (since it unconditionally marks the execution phase as async), which, by spec, is very explicitly fully synchronous - this is potentially witnessable with a late-loaded module graph or by a loader!)

[bcomnes]

I don't think it is benefiting our users continuing to throw here to carve out the future design space at the cost of today's UX

I challenge the assertion that this change improves user experience. It confuses and obfuscate whats going on. We already have people confused at capabilities this change hints at in this very thread , even though it fails to provide whats hinted at. (e.g. people thinking that they can simply prefix await in front of require(foo.mjs) behaving as import(foo.mjs) in the the top of files where they normally are requireing packages, and that invisibly switching out require for import and is a big shortcut to get there. This of course, won't work because you can't top level await in a cjs file, and re-writing require to import.then() requires a non-trivial restructuring of the code at that point, nullifying the already thin benefits of a shortcut here.

I don't personally see there being a future where we can actually support synchronous require of ESM

Blowing the opportunity to land sync loading of esm from cjs in #30891 in favor of this solution would really be an incredible loss. The possibility of writing a single export esm module that still works in a cjs context (even with caveats) that doesn't prescribe significantly rewriting the cjs side would provide massive value to the mountain of Node.js code in existence in addition to new code being written that still has a high likelihood of interacting with older cjs codebases (by not requiring the high maintenance cost of transforming and exporting two separate code bases, and allowing for the consumption of other ESM only modules without violating module boundaries as dual export requires in that scenario). Unless @weswigham, who graciously provided a working implementation of sync loading esm in cjs, is giving up on the problem, I am not giving up on it either.

@MylesBorins Because I'm criticizing this change, I would also clarify I'm appreciative of your time and energy overall and this criticism is limited to the idea.

[bmeck]

(which is still planned to be done...? @bmeck )

The loaders WG is actively talking about it still but are doing other refactors first. I've made 2 PRs in the past and am not actively working on a new one but would still think this is the only viable path given I'm also adding https: support in a different PR and don't think synchronous network requests are likely.

[GeoffreyBooth]

Except it still doesn’t work, because there is no top-level await in CommonJS

Yes . . . I think really what users want is require('esm') that works as current require does: synchronous and top-level. Anything short of that feels like it will make the present situation even worse.

[MylesBorins]

So it seems to me that the general sentiment is:

• that this feature might marginally improves that status quo, but that does not have consensus (some think it makes it worse)
• the behavior implemented in this PR exists in userland which implies it might be worth doing, but that also means that people are able to work around the lack of this feature
• Existing widely used code such as Mocha makes this change Semver-Major
• There may still be a path to a sync "ish" require('esm')

I think with all of the above it might make sense to abandon this change today and re-explore prior to Node.js 18 if we want to really do this. Thoughts?