nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
106.76k stars 29.13k forks source link

Cannot use mjs file synchronously in commonJS without await / .then() #33605

Closed Raynos closed 4 years ago

Raynos commented 4 years ago

What steps will reproduce the bug?

Require an mjs file.

const isPromise = require('./third-party-is-promise.mjs')

console.log(typeof isPromise)

Example https://gist.github.com/Raynos/2c6bb0a8537dbf7d464a9b85813cd3b1

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior?

I can load an ESM module from a commonJS file.

What do you see instead?

~/tmp/temp123
$ node broken.cjs 
internal/modules/cjs/loader.js:984
    throw new ERR_REQUIRE_ESM(filename);
    ^

Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: /home/raynos/tmp/temp123/third-party-is-promise.mjs
    at Module.load (internal/modules/cjs/loader.js:984:11)
    at Function.Module._load (internal/modules/cjs/loader.js:879:14)
    at Module.require (internal/modules/cjs/loader.js:1026:19)
    at require (internal/modules/cjs/helpers.js:72:18)
    at Object.<anonymous> (/home/raynos/tmp/temp123/broken.cjs:1:19)
    at Module._compile (internal/modules/cjs/loader.js:1138:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1158:10)
    at Module.load (internal/modules/cjs/loader.js:986:32)
    at Function.Module._load (internal/modules/cjs/loader.js:879:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12) {
  code: 'ERR_REQUIRE_ESM'
}

Additional information

I would like to import an ESM synchronously from a commonJS file.

Node added child_process.execSync() a while ago but that used to not be possible.

It would be really nice if node had some kind of importSync() feature.

I also tried

const isPromise = import('./third-party-is-promise.mjs')

console.log(typeof isPromise)
console.log('then method', typeof isPromise.then)

This returns a promise instead of the code which is not what I want

Raynos commented 4 years ago

I see from a related PR that a function promiseWait exists.

If I can just call require.promiseWait(import('./esm.mjs')) then that would also allow me to get a promise for a ESM module using import and then just blocking wait until the promise resolves.

We don't have to do anything "special" with require we just expose a process.awaitSync(p) function.

bmeck commented 4 years ago

Relevant discussion here https://github.com/nodejs/modules/issues/454

GeoffreyBooth commented 4 years ago

And there's a PR at #30891, but it has so many issues that that's why https://github.com/nodejs/modules/issues/454 was opened. It's not clear that the issues raised will be addressable; or in plainer terms, require of ESM might not be possible.

ESM code can be loaded into a CommonJS module via import(). https://nodejs.org/api/esm.html#esm_import_expressions

Raynos commented 4 years ago

Ok, I would like to importSync or require a ESM module and if it has a top level await you can just throw an exception saying "Some ESM has top level await; not supported"

Raynos commented 4 years ago

I've also found a userland solution

const syncify = require('@snek/syncify')
const isPromise = syncify(import('./third-party-is-promise.mjs'))

There are native modules on npm that implement awaitSync() as a C++ function.

devsnek commented 4 years ago

Those who use @snek/syncify are doomed to a life of sadness

GeoffreyBooth commented 4 years ago

More realistically, if you're looking for a way to "bottle up" promises and resolve them synchronously, you should look into node-fibers and its use in the framework Meteor. That's at least one example of doing so in a widely used framework that has many production apps. Meteor (at least currently) only supports ESM by transpiling it to CommonJS, however; it's unclear whether it can ever support native ESM along with node-fibers. Based on the discussion in https://github.com/nodejs/modules/issues/454, such a thing might not be possible, or possible without violating spec (which is something that Node can't do).

bmeck commented 4 years ago

The ECMAScript spec does actually provide data on if top level await is used: https://tc39.es/proposal-top-level-await/#sec-cyclic-module-records in a new internal [[Async]] slot, however this is set for more than just top level await; all WASM modules have that as true due to their initialization lifecycle as I understand. This internal slot is not currently reflected in V8's public API though to my knowledge. I think we could review this if/when it is exposed as a means to help transitioning.

Raynos commented 4 years ago

Could you explain more about the limitations of loading a WASM module synchronously ?

I think require(mjs) and require(wasm) are two completely different beasts.

I would be fine with a require(mjs) that fails on transitive dependencies that use Top level await and WASM.

The fast majority of the uses cases for ESM is trivial code that doesn't do anything special with top level await or WASM.

bmeck commented 4 years ago

@Raynos I am not as clear on the reasoning, but they always initialize async in module form. Someone more vested in their design/implementation would be more knowledgable.

devsnek commented 4 years ago

wasm esm modules take advantage of off-thread compilation and things of that nature, using wasm's async apis.

Raynos commented 4 years ago

I was looking at using ESM again and sharing some ideas on twitter ( https://twitter.com/Raynos/status/1285109697662640130 ).

Having importSync() available would help migrate to ESM without breaking all my commonJS users.

devsnek commented 4 years ago

so, we're not ever going to have importSync. cjs to esm interop is a problem we'd like to solve but it wouldn't be in this manner, and I think we already have other issues tracking this more generally, so I'm going to close this.

Raynos commented 4 years ago

Could you link to the more general issue for using esm from cjs ? I just did a search and could not find anything.

devsnek commented 4 years ago

@Raynos https://github.com/nodejs/modules/issues/454

Raynos commented 4 years ago

Oh it's not in this repository. And there's this PR which was already referenced ( https://github.com/nodejs/node/pull/30891 )