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.