Fix: Load CommonJS .eslintrc.js files within a "type": "module" package scope (fixes #12319)

[GeoffreyBooth]

Fixes #12319, where CommonJS .eslintrc.js files inside a project where the user has added "type": "module" to their package.json were throwing an error.

Sorry for the lack of tests, but please feel free to complete this PR if you like this approach to solving the problem. It may become moot if the error is removed in Node (see nodejs/modules#389 and nodejs/node#29732).

[kaicataldo]

Thanks for the PR. As mentioned here, we ask that changes to core go through the RFC process outlined here](https://github.com/eslint/rfcs).

[GeoffreyBooth]

This isn’t an enhancement like #12321. If you look at the changes in this PR, it’s narrowly targeted to fix #12319 and nothing more.

[mysticatea]

I agree that this is not an enhancement. But we need tests to confirm what PR fixed and because the ERR_REQUIRE_ESM doesn't happen in stable environments, I feel that this PR is too early to be merged.

[GeoffreyBooth]

ERR_REQUIRE_ESM happens in Node 12.11.0 stable, without any experimental flags.

[mysticatea]

I'm seeing that the error has been moved behind an experimental flag in nodejs/node#29732.

After re-look, I think that we can replace importFresh() by requireFromString() completely if requireFromString() doesn't break existing configs.

[GeoffreyBooth]

I’m seeing that the error has been moved behind an experimental flag in nodejs/node#29732.

Yes, for now. I’m on the Node team working on this, and we haven’t decided what to do about this when --experimental-modules is unflagged, which will probably happen in October 2019. The error could just come back, it might turn into a warning, or it could be removed.

Ultimately, the error was added for a reason: if a user adds "type": "module" to their package.json, they’re telling Node (and any other tools that respect "type") that the .js files in that package scope should be treated as ES modules. So having .eslintrc.js be a special case where it alone is treated as CommonJS while all the .js files around it are parsed as ES modules . . . will confuse some users. You should still merge in #12321 as well, and update the docs to recommend people start using the .cjs extension for a CommonJS .eslintrc.cjs, as that’s a pattern that doesn’t involve inconsistencies or cheating to bypass Node’s error.

After re-look, I think that we can replace importFresh() by requireFromString() completely if requireFromString() doesn’t break existing configs.

Sure! I don’t have the time to do that kind of testing; thanks for investigating.

[mysticatea]

@GeoffreyBooth Thank you for working on that!

I found some questions while I play with ES modules, may I ask you?

My goal is to find the way that imports modules with import() syntax without cache, like importFresh(). I'm using v12.11.0 on Windows.

As the document,

require.cache is not used by import. It has a separate cache.
https://nodejs.org/api/esm.html#esm_no_code_require_cache_code

Modules will be loaded multiple times if the import specifier used to resolve them have a different query or fragment.
https://nodejs.org/api/esm.html#esm_url_based_paths

From the above, I guess

• we cannot remove the import cache.
• we use package-name?q=1234 to import modules freshly.

But I couldn't find a way to use query strings with a package name.

    await import("x-esm?q=2") //→ Cannot find package 'x-esm?q=2'
    await import("x-esm/?q=3") //→ Cannot find module C:\Users\t-nagashima.AD\dev\eslint\node_modules\x-esm\
    await import(require.resolve("x-esm") + "?q=4") //→ Cannot find module /\Users\t-nagashima.AD\dev\eslint\node_modules\x-esm\index.js

How should I use query strings with a package name? (or is there another way to import packages without cache?)

---

I found an odd behavior about import(cjs).

// test.js (cjs)
;(async () => {
    // console.log("ESM")
    await import("x-esm") //→ Found (this package just contains `console.log("***")`)
    await import("./node_modules/x-esm/index.js?q=0") //→ Found and `x-esm` re-ran as expected
    await import("./node_modules/x-esm/index.js?q=1") //→ Found and `x-esm` re-ran as expected

    console.log("CJS")
    await import("x-cjs") //→ Found (this package just contains `console.log("***")`)
    await import("./node_modules/x-cjs/index.js?q=0") //→ Found but `x-cjs` didn't re-run
    await import("./node_modules/x-cjs/index.js?q=1") //→ Found but `x-cjs` didn't re-run
    
    console.log("remove 'require.cache'.")
    delete require.cache[require.resolve("x-cjs")]
    await import("x-cjs") //→ Found but `x-cjs` didn't re-run
    await import("./node_modules/x-cjs/index.js?q=0") //→ Found but `x-cjs` didn't re-run
    await import("./node_modules/x-cjs/index.js?q=1") //→ Found but `x-cjs` didn't re-run
    await import("./node_modules/x-cjs/index.js?q=2") //→ Found and `x-cjs` re-ran as expected
    await import("./node_modules/x-cjs/index.js?q=3") //→ Found but `x-cjs` didn't re-run
    console.log("remove 'require.cache'.")
    delete require.cache[require.resolve("x-cjs")]
    await import("x-cjs") //→ Found but `x-cjs` didn't re-run
    await import("./node_modules/x-cjs/index.js?q=0") //→ Found but `x-cjs` didn't re-run
    await import("./node_modules/x-cjs/index.js?q=1") //→ Found but `x-cjs` didn't re-run
    await import("./node_modules/x-cjs/index.js?q=2") //→ Found and `x-cjs` didn't re-run
    await import("./node_modules/x-cjs/index.js?q=3") //→ Found but `x-cjs` didn't re-run
    await import("./node_modules/x-cjs/index.js?q=4") //→ Found and `x-cjs` re-ran as expected
})().catch(error => {
    console.error(error)
    process.exitCode = 1
})

If the import source is ESM, it works fine. But if the import source is CJS, it's odd.

• The CJS package didn't re-run regardless of query strings.
• It creates require.cache entry (but the entry doesn't have parent property, and module.children is still empty).
• If I removed the require.cache entry, then still import cache exists and the CJS package re-ran when it imports the CJS package with a new query string.

Actually, shouldn't import(cjs) create require.cache entry?

---

This is off-topic, but I have a concern.

    await import("x-esm/") //→ Cannot find module C:\Users\t-nagashima.AD\dev\eslint\node_modules\x-esm\

Since built-in punycode module has been deprecated, we use third-party punycode package. But require("punycode") still imports built-in punycode module. Therefore, we use require("punycode/") to use the third-party punycode package.

Cannot we use this workflow in ES modules? Is there the formal way to import third-party packages which have the same name as a built-in module?

[GeoffreyBooth]

Hi @mysticatea, I’ll try to answer your questions, but I’m certainly not an expert. I’ve asked other members of the modules team to chime in if they can.

How should I use query strings with a package name? (or is there another way to import packages without cache?)

I think you’d have to read the package’s package.json to get its "main" field, and then add a query string to that. So like if x-esm’s main is index.js, you would do import('x-esm/index.js?q=1234'). You need to use createRequire (or fs.readFile/fs.readFileSync) to read the package.json. This is just supposition on my part though, you might want to open an issue against Node in case this is a bug.

I found an odd behavior about import(cjs).

• The CJS package didn’t re-run regardless of query strings.

Yes. For CommonJS, the specifier (the string passed to require or import()) is basically a file path, whereas for ES modules, the specifier is treated as an URL. Since query strings aren’t used in file paths, they have no effect in CommonJS. Even though import() is used, if the thing being loaded is CommonJS, the CommonJS loader is used and the specifier follows CommonJS/require semantics.

Actually, shouldn’t import(cjs) create require.cache entry?

Probably? Though maybe not because import() isn’t require? I dunno.

require("punycode") still imports built-in punycode module. Therefore, we use require("punycode/") to use the third-party punycode package. Cannot we use this workflow in ES modules? Is there the formal way to import third-party packages which have the same name as a built-in module?

This is an edge case that I don’t think anyone’s considered, and your use of a trailing slash there seems like an unsafe workaround. It might be a good idea to publish a wrapper package like punycode-wrapper that just imports and reexports the third-party punycode, and then you can simply require('punycode-wrapper')/import('punycode-wrapper'). You could also try referencing the full relative path, like import punycode from './node_modules/punycode/punycode.js'.

[mysticatea]

Thank you for your answer!

I think you’d have to read the package’s package.json to get its "main" field, and then add a query string to that. So like if x-esm’s main is index.js, you would do import('x-esm/index.js?q=1234'). You need to use createRequire (or fs.readFile/fs.readFileSync) to read the package.json. This is just supposition on my part though, you might want to open an issue against Node in case this is a bug.

I got it. It's great if ESM version of require.resolve function exists. For now, I can use import(`file:${require.resolve(name)}?${uniqueQuery}`).

Probably? Though maybe not because import() isn’t require? I dunno.

I have opened an issue: nodejs/node#29812

This is an edge case that I don’t think anyone’s considered, and your use of a trailing slash there seems like an unsafe workaround.

Indeed, it's an edge case. But also shims for browsers often have the same name as built-in modules, so I guess it's not a rare case. I hope the standard way to load such a package without the knowledge of the internal file structure of the packages.

[xiaoxiangmoe]

Within a "type": "module" package scope, Node.js can be instructed to interpret a particular file as CommonJS by naming it with a .cjs extension

https://nodejs.org/dist/latest-v13.x/docs/api/esm.html

Maybe using .eslintrc.cjs as config file will be better.

---

Another example: babel/babel#10599

[kaicataldo]

How do we want to proceed with this?

[mysticatea]

I mildly oppose this change. Because this will make hard to support ESM config files in the future. If we leave the ERR_REQUIRE_ESM error, we can update to use import() in the future safely. On the other hand, if we hide the ERR_REQUIRE_ESM error, to update to use import() will be a breaking change.

[kaicataldo]

I agree with @mysticatea. I don't think this is something we should do right now, since we're actively working towards supporting ESM in core.

[kaicataldo]

I'm going to go ahead and close this PR since the team has decided not to go forward with this proposal. We appreciate your contribution!