12 LTS backporting of ERR_REQUIRE_ESM

[guybedford]

@MylesBorins did some amazing work this weekend and got the full modules implementation (minus the unflagging of --experimental-modules) fully backported to 12 staging (https://github.com/nodejs/node/tree/v12.x-staging). Opening this issue to discuss one of the backport details on the module agenda this week.

Specifically - should we flag the ERR_REQUIRE_ESM errors that are thrown for .mjs and scopes? These result in library bugs like the following respectively - standard-things/esm#855, eslint/eslint#12319. Alternatively should we turn these errors into warnings?

[ljharb]

I believe the eslint one has been fixed?

Either way they are errors, and the user who set type module, while using a tool that can’t handle it, is fully in control of being able to revert that change pending the tool adjusting its behavior. It seems like the errors are desirable.

[evanplaice]

Can confirm. The PR I submitted to add .cjs as a config target in ESlint landed a couple of weeks ago.

I also agree 100%. Better to error sooner so we know what needs to be patched.

[GeoffreyBooth]

Just so I’m clear, aren’t these errors already unflagged on 12? Or they were briefly unflagged on 12 before we put them behind --experimental-modules?

[giltayar]

Please don't turn the error off, or turn it into a warning.

Mocha relies on that error in its support of ESM: it first require-s the module, and if it get ERR_REQUIRE_ESM, it switches to import.

[guybedford]

Gil would it work to disable the .mjs error only? You could still detect the file extension manually. My concern there is I think the mjs ERR_REQUIRE_ESM still breaks require(‘esm’). We should probably double check this for certain though.

…

On Tue, Jan 14, 2020 at 11:25 Gil Tayar ***@***.***> wrote: Please don't turn the error off, or turn it into a warning. Mocha relies on that error in its support of ESM: it first require-s the module, and if it get ERR_REQUIRE_ESM, it switches to import. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#468?email_source=notifications&email_token=AAESFSTTPV2QPKPGMCDX5HLQ5WAJ7A5CNFSM4KGM5CL2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEI342AQ#issuecomment-574082306>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAESFSS3VQXOX55SHDACW3TQ5WAJ7ANCNFSM4KGM5CLQ> .

[jkrems]

Could we do the following:

1. Detect SyntaxError in the compile and only throw ERR_REQUIRE_ESM if there is one.
2. Warn if it succeeds.

It wouldn't break mocha's approach while not breaking improperly set up (but currently "working") projects on existing versions of node 12..?

EDIT: Mostly asking so we have considered the option. Not sure myself if that complexity is good, especially given how many different cases our decision tree already has.

[ljharb]

No, because that would allow an ambiguous file that could be either parse goal to succeed in the wrong goal.

[guybedford]

I've just tested require('esm') against the v12.x-staging branch and can confirm that it is working fine with no bugs both without and without the --experimental-modules flag.

The issue only shows up when you explicitly set "type": "module" in a package scope (and try to load a CommonJS file from a .js extension).

[MylesBorins]

Removing agenda label. Was there anything actionable as a follow up to today's discussion?

[guybedford]

Nothing actionable, just for us to be aware of.

[giltayar]

@guybedford - even though closed (just saw this), note that I check if the extension is .mjs and always use import on that. So, yes, if you disable the error when require-ing .mjs, that would be fine by me.

[guybedford]

@giltayar thanks for the update here. I did test this again against require('esm') and it was actually fine. In addition the .mjs error can be disabled in userland by overridding require._extensions['.mjs'].

We agreed in the meeting we were ok with these errors being backported for correctness. They may well bite some early adopters, but that is the risk of early adoption unfortunately.

[aditya369thalluri]

@guybedford

I am working on the Nuxt Js project and trying to import vuelayers module. Getting the below error screen.

I know it is because of ES6 syntaxing and tried keeping "type:module" in package json. Still that does not and say's that require() of ES modules is not supported.

Can you please help me in solving this

[ljharb]

@aditya369thalluri type: module only controls whether .js files are ESM or CJS (as opposed to just .mjs, which are always ESM, and .cjs which are always CJS). Either way, you can't require native ESM, you can only import CJS.

[jkrems]

I know it is because of ES6 syntaxing and tried keeping "type:module" in package json. Still that does not and say's that require() of ES modules is not supported.

Adding type:module to package.json can end up more misleading than helpful. Because it can lead to tools like nuxtjs that may not properly support native modules, blindly trying to to require those .js files, resulting in confusing errors. In many cases it's more helpful to keep .mjs and properly understand why they aren't picked up. More often than not it will be because native modules aren't actually supported in that context.

[jkrems]

For future reference: It looks like nuxtjs only supports ESM for modules from this PR description. It's not clear to me that general application code can use ESM. Not sure if there's a story on their side, looks like vue components don't really make a difference between require and import.

[aditya369thalluri]

What could be the solution to this? I am just trying to import a package and errors "cannot use import statement outside a module". It is not happening for all packages. It is only for this OpenLayers package. I am still able to import other packages.

[aditya369thalluri]

https://github.com/ghettovoice/vuelayers/
This is the OpenLayers package I installed and trying to import. Could there be something wrong with this?

[jkrems]

@aditya369thalluri It looks like vuelayers is taking putting pseudo modules into main:

  // Code generated from rollup --es but using .js so likely not intended to run as modules
  "main": "./lib/index.js",
  "module": "./lib/index.js",
  // Code generated from rollup --umd, likely intended to run as a script or CommonJS
  "unpkg": "./lib/index.umd.js",

Looks like your nuxt setup is trying to load the main as CommonJS (which is fairly reasonable) but isn't compiling away the import-sugar to make it work in node. It may be worth opening an issue either in vuelayers ("hey, don't put files that need compilation to work into main") or nuxt ("hey, please transpile away the import sugar in deps before trying to run files in node").

P.S.: A way to phrase the vuelayers issue is "please support SSR using this code". Their approach of assuming that import sugar gets compiled away before running the code is perfectly valid if they only target bundlers. :)