Modules: Warn if ESM is used in .js

[xtuc]

I expected that many users will be confused by .mjs and ESM.

Here's an example:

root
├── a.js

export const a = 1;

└── index.mjs

import {a} from "./a.js"
console.log(a);

When running this with node --experimental-modules index.mjs, the error is the following:

(node:9967) ExperimentalWarning: The ESM module loader is experimental.
file:////test/index.mjs:1
import {a} from "./a.js"
        ^
SyntaxError: The requested module './a.js' does not provide an export named 'a'
    at ModuleJob._instantiate (internal/modules/esm/module_job.js:89:21)

While importing ./a.js is not an error (since CJS interop), it's clear that the user intended to use ESM. It would make sense to warn the user.

Note that .js/.mjs is not clear in tooling yet (Babel), so it could end up being generated.

(node v10.4.1)

[devsnek]

they've already got a syntax error though, that's pretty clear imo

it's clear that the user intended to use ESM.

I don't think it is, perhaps they were intending to write commonjs and had a brainfart, I know that's something I would do. trying to guess user intent always seems fragile to me.

[goto-bus-stop]

they've already got a syntax error though, that's pretty clear imo

i would expect a syntax error being thrown from a.js, but it seems like that gets silenced by the esm error. the error that is shown currently is the same as if you were importing an ES module that does not have an export named a.

i'd expect the same error as the one you get when running a.js directly:

$ node --experimental-modules a.js
/tmp/a.js:1
(function (exports, require, module, __filename, __dirname) { export var a = 1
                                                              ^^^^^^
SyntaxError: Unexpected token export

[devsnek]

oh whoops I didn't notice that. that seems like a bug. but assuming that the correct error showed I think this would be fine.

/cc @bmeck maybe this is related to not using v8's errors? 😅

[devsnek]

ok after looking into this codewise i have to flip-flop yet again. the error message is correct. we declare the exports of cjs modules before we evaluate them, and then while instantiating, the loader sees that the expected default export of the cjs module doesn't match the asked-for named a import.

if you change index.mjs to ask for the default export, you get the error you expect:

$ node-dev --experimental-modules index.mjs
(node:73883) ExperimentalWarning: The ESM module loader is experimental.
/Users/gus/Desktop/misc/nodejs/node/a.js:1
(function (exports, require, module, __filename, __dirname) { export const a = 1;
                                                              ^^^^^^

SyntaxError: Unexpected token export

[jkrems]

@devsnek But we could, in theory, detect that the user tried to get a non-default export from a CJS module and give a helpful error message? Not sure if that's a worthwhile feature while in experimental and things are in flux but could be good to keep in mind.

[devsnek]

@jkrems yeah i'm looking into that now, its just a bit of spaghetti at the moment :P

[targos]

In theory we could but I think we would need a hook that doesn't exist in V8 (get the list of exports are being requested by the user)

[xtuc]

Should I move this issue on node/node? since it's a bug

[devsnek]

@xtuc nah, this turns out not to be a bug, still worth discussing here as a behavioural change.

[ljharb]

Someone doing import { Component } from ‘react’ isn’t trying to import ESM, they’re trying to import react. The most helpful error messages here are “there’s no named import Component there”, bt also “the default export has a Component property, perhaps you meant to destructure that separately?”

The user shouldn’t have to know the module format and that’s not actually relevant to what they’re trying to do.

[bmeck]

@ljharb that would require us to get V8 to expose what imports are currently requested, also would need to take into account that full namespace exports wouldn't be as simple as that since they are not listing names to import.

[ljharb]

Fair, I’m just voicing where i think the improvements should come from - i don’t think reporting the module format is a good idea.

[MylesBorins]

Is there still more to discuss with this issue or can it be closed?

[bmeck]

This can be moved to fork, but still needs implementation work.

[MylesBorins]

Closing this as there has been no movement in a while, please feel free to re-open or ask me to do so if you are unable to.