Script-based configuration files in "type": "module" packages

[guybedford]

Tracking issue specifically for the ESLint issue that was raised here - eslint/eslint#12319.

Tooling often rely on configuration scripts. Examples include .eslintrc.js, karma.conf.js, webpack.config.js, babel.config.js to name a few.

These tools typically load those configuration files with a require.

When an existing project with such a configuration adds "type": "module" to the package.json, they get the ERR_REQUIRE_ESM dual hazard error that we added recently in nodejs/node#29492.

The ideal upgrade path is to have the tools support using import() for loading their configurations. Alternatively they could support .cjs as well.

We should probably reach out with this information to tools and get further feedback as well.

[coreyfarrell]

This is a major breaking change for nyc and tap. Both supported testing ES modules using the esm module or with @babel/register. Now running nyc and/or tap on node.js 12.11.0 with "type": "module" causes a complete failure of testing, both fail with Error [ERR_REQUIRE_ESM]: Must use import to load ES Module.

CC @bcoe @isaacs

[Jamesernator]

Perhaps once again consider a pragma-based approach as an escape hatch (e.g. "use commonjs" or "format:commonjs").

While there's a little burden on package users it gives them a functioning solution until the tools can update.

[guybedford]

I've added nodejs/node#29732 to put this check back behind the --experimental-modules flag, instead of being on by default, while we discuss it further.

[GeoffreyBooth]

eslint/eslint#12319 (comment)

[devsnek]

i'd like to really strongly advocate that we look for solutions that reduce the occurrences of package.json files. they slow loading down quite a bit (which is why we traditionally never supported any fields besides "main") and are obviously incompatible with non-node runtimes.

[coreyfarrell]

The ideal upgrade path is to have the tools support using import() for loading their configurations.

https://nodejs.org/api/esm.html#esm_code_import_code_expressions suggests that dynamic import() can be used from CommonJS but this is not always true. In node.js 8 if --experimental-modules is not enabled then import(specifier) is a SyntaxError. It's always a SyntaxError in node.js 6 (I know node.js does not support 6.x but some tools still do). In node.js 10/12 it rejects asynchronously if --experimental-modules is not enabled. Could the dynamic import function be conditionally exposed so it could be grabbed from say require.dynamicImport? Obviously dynamicImport should only be defined if --experimental-modules is enabled or unflagged. This would allow tools to detect the feature without risk of SyntaxError or having to deal with asynchronous 'Not supported' rejections.

This does not resolve the issue with test runners executing scripts, I'm not sure how that can be solved.

[devsnek]

it's a syntax error the same way trying to use numeric separators in node 8 is a syntax error. i don't think we should try to circumnavigate that.

[coreyfarrell]

Numeric separators throwing SyntaxError in node 8 is not an issue because not using numeric separators is never a SyntaxError. A way to load *.config.js is needed, I'm just pointing out that the suggestion to use import() is not practical as it is a syntax error by default in all current versions of node.js.

[devsnek]

import is syntax because it's not just a function call, it has side-channel data about where it's called from that is needed to resolve import requests.

aside from that though, one of our goals is to make node code more interoperable by default, and adding some new import api would interfere with that.

[ljharb]

I think the point @coreyfarrell is trying to make (that i agree with) is that since most ecosystem packages support "not just the latest node", it is simply not a viable workaround to recommend use of import() - additionally, since older nodes won't support requiring of .cjs, that is also not a sufficient workaround.

[bmeck]

@ljharb require loads any unknown extension as CJS, .cjs works for all versions of node as long as someone doesn't clobber require.extensions['.cjs'].

[ljharb]

Thanks for the correction; in that case, that's probably the best workaround for the time being.

[coreyfarrell]

import is syntax because it's not just a function call, it has side-channel data about where it's called from that is needed to resolve import requests.

aside from that though, one of our goals is to make node code more interoperable by default, and adding some new import api would interfere with that.

It's true that tools can add support for loading config from .cjs files. This does not help move forward. Let's say that --experimental-modules gets unflagged in node.js 12.12.0 (first LTS). It will be years before these tools drop support for everything older than v12 LTS. I'd like to see these tools support ESM configuration files but without a non-syntax method of importing ESM this will not happen.

Would it reduce your concerns if module.createRequire never provided the dynamicImport method? That way require.dynamicImport(filename) would only be available from CJS, never ESM.

[nicolo-ribaudo]

@coreyfarrell I'm thinking about this for Babel:

async function tryLoad(path) {
  try {
    return require(path);
  } catch (e) {
    try {
      return await require("./import")(path);
    } catch {
       throw e;
    }
  }
}


// import.js
module.exports = p => import(p);

By doing so, the file containing import isn't loaded in old node.

[devsnek]

for better or worse, that is what i would suggest, assuming eval is out of the question.

[GeoffreyBooth]

@devsnek Is there some way to evaluate the ESM source code as a string? Like (psuedocode):

const configSource = readFileSync('./.eslintrc.js', 'utf8');
const config = somehowEvalAsESM(configSource);

Reading the file as a string dodges the “load as CommonJS or ESM” issue. The tool would then need to figure out somehow whether to eval as CommonJS or as ESM, perhaps via something like nodejs/node#27808; and use vm.Script.runInThisContext or the equivalent ESM method for evaluating; right? Is there some way to do the ESM version synchronously?

[devsnek]

vm.SourceTextModule, although it's still experimental and I'm about to open a pr reworking the API of it. Evaluation is always exposed as async though, since TLA is about to land. I think requiring/evaling import() is the way to go

[GeoffreyBooth]

vm.SourceTextModule, although it’s still experimental and I’m about to open a pr reworking the API of it. Evaluation is always exposed as async though, since TLA is about to land. I think requiring/evaling import() is the way to go

If you could rework the SourceTextModule API to be as simple as runInThisContext, i.e. letting me eval ESM code in a one-liner, I would be most grateful 😄

Aside from that, though, are you saying that while I can synchronously eval CommonJS strings via runInThisContext, there’s no way (and won’t be some way in the future) to synchronously eval ESM strings?

[devsnek]

there’s no way (and won’t be some way in the future) to synchronously eval ESM strings?

correct. all the exposed edges of esm graphs are async.

[GeoffreyBooth]

correct. all the exposed edges of esm graphs are async.

Okay, so thinking more outside the box then the only ways to achieve this are to use node-fibers to wrap the async calls in a Future or to use execSync/spawnSync to spawn a separate Node process to eval the ESM string and return its output. The former requires a compatible platform and the latter seems like it would be a heavy performance hit, so they’re far from ideal; but it’s not impossible.

[weswigham]

I'd like to see these tools support ESM configuration files but without a non-syntax method of importing ESM this will not happen.

So you'd like require to simply support loading esm, I hear.

[coreyfarrell]

So you'd like require to simply support loading esm, I hear.

No, I was just suggesting for node.js to expose import() as a normal function. require cannot be used because it is synchronous. I'll probably do something similar to what @nicolo-ribaudo suggested.

[GeoffreyBooth]

I was just suggesting for node.js to expose import() as a normal function.

import() is defined in the language, it’s not a function like require: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import#Dynamic_Imports. Think of it more like a keyword; you can’t override it the way you can hijack require.

[weswigham]

Except such an API breaks every time the esm loader gains support for, eg, loading json. I'd think phrasing it in terms of the cjs loader would handily work around that.

[jkrems]

Except such an API breaks every time the esm loader gains support for, eg, loading json.

Can you elaborate which API you're talking about and in what way it breaks?

[weswigham]

If you have an isImport("./f.json") style API, it's going to change its result (potentially) when/if esm supports json imports. And it's definitely confusing for builtins, since they... Are?... importable but also requireable. I just think that since the cjs loader already classifies these things (to delegate to extension handlers), exposing that classification would be pretty useful and follow pretty well from how the resolver works. Like, if you can say "this result means this extension handler would be used", that'd be nice. And potentially generalize to custom extension handlers.

[coreyfarrell]

Node.js 13.2.0 emits a warning upon attempt to use require('/path/to/filename.js') for a "type": "module" location. This is a problem for packages which use ERR_REQUIRE_ESM to detect the need to perform import(). I'm not clear the value of the warning when we already have a throw?

[jkrems]

There was a blocking issue with ESLint configs where ESLint only supports .js and always requires it. We decided that we couldn't make it throw yet but wanted to at least warn. It's not a great state, agreed. If you can use import() it may actually be better to default to import() and only use require as a fallback.

[nicolo-ribaudo]

If detection via ERR_REQUIRE_ESM doesn't work anymore, is there any alternative?

[GeoffreyBooth]

If detection via ERR_REQUIRE_ESM doesn't work anymore, is there any alternative?

try {
  await import(pathToFilename);
} catch (exception) {
  require(pathToFilename);
}

import() can accept both ES module and CommonJS sources, whereas require supports only CommonJS. Therefore it would seem more probable that import() has success first before require does. You still need the require fallback for certain types that import() doesn't support, like JSON.

[nicolo-ribaudo]

Ok currently I'm doing something conceptually similar to this (don't judge me for the sync/async thing, I promise that the exposed API is nicer 😆)

function loadConfig(path, isAsync) {
  let config;

  try {
    config = require(path);
    if (isAsync) config = Promise.resolve(config);
  } catch (e) {
    if (e.code !== "ERR_REQUIRE_ESM") throw e;
    if (!isAsync) throw new Error("Cannot use esm synchronously");
    config = import(path);
  }

  return config;
}

I don't think that I could do the same without "manually" looking up the package.json file 🤔

[jkrems]

How about:

function loadConfig(path, isAsync) {
  if (isAsync) return import(path);

  try {
    return require(path);
  } catch (e) {
    if (e.code !== "ERR_REQUIRE_ESM") throw e;
    throw new Error("Cannot use esm synchronously");
  }
}

[coreyfarrell]

Interesting note, the following code executed by CJS does not produce any warning or error in node.js 13.2.0:

import('./nyc.config.mjs')
	.then(console.log)
	.catch(e => console.error(e.message));

I suspect it is a bug that it doesn't emit ExperimentalWarning: The ESM module loader is experimental.? It would be fine for nyc to cause this warning if the config was actually .mjs or .js with a type: 'module' but it cannot cause the experimental warning upon load of a CJS format.

Beyond this if we were to use import() first I would probably restrict that to 13.2.0+. When older versions are run without --experimental-modules we would have to detect the async Not Supported rejection while avoiding spurious warnings. This could be solved if we had a safe, silent and reliable feature detection API. For example if (require('module').esmSupported) { ... } to determine if it's import() is supported.

@GeoffreyBooth using the fallback method has issues with error handling. Your example code would swallow the true error if pathToFilename were ESM that had an exception during load, say the module contained if (envIsBad) { throw new Error('Must fix X in the environment'); } at the top level. The following example might work, the ERR_UNKNOWN_FILE_EXTENSION check ensures the correct exception is thrown for JSON parse errors.

try {
  return await import(pathToFilename);
} catch (importError) {
  try {
    // XXX if a CJS module throws this will cause it to
    // be evaluated a second time.
    return require(pathToFilename);
  } catch (requireError) {
    if (importError.code === 'ERR_UNKNOWN_FILE_EXTENSION') {
      throw requireError;
    } else {
      throw importError;
    }
  }
}

Thankfully I'm not facing the same restriction as babel, config load for the next version of nyc will always be async.

[nicolo-ribaudo]

@coreyfarrell I think that your suggestion has a problem: if the config is a commonjs file which throws an error, anything before that error will be executed twice.

[jkrems]

For example if (require('module').esmSupported) { ... } to determine if it's import() is supported.

I think that's a great idea!

[coreyfarrell]

@nicolo-ribaudo good catch, I edited my code to include a comment so no-one uses my example unaware. For my config loader I'm using fs.readFile to load JSON so I'll probably just drop the require() fallback entirely (sorry I know this doesn't help you).

@jkrems should I create a separate issue to get feedback on adding a detection flag?

[GeoffreyBooth]

For example if (require('module').esmSupported) { ... } to determine if it's import() is supported.

I think that's a great idea!

function isDynamicImportSupported() {
  try {
    new Function("import('')");
    return true;
  } catch (exception) {
    return false;
  }
}

Returns true in Node 13.2.0, false in Node 8.16.1 (just happened to be what I tested it in).

[coreyfarrell]

@GeoffreyBooth your example returns true in node.js 12.11.0 even without --experimental-modules. What I'm asking for would return false in that case.

[jkrems]

@GeoffreyBooth Also, even doing the test already may print a warning I assume. I don't think there's a clean, no-side effects, way of testing if ESM could be imported. Especially not if that test is supposed to be sync.

[GeoffreyBooth]

@GeoffreyBooth Also, even doing the test already may print a warning I assume. I don't think there's a clean, no-side effects, way of testing if ESM could be imported. Especially not if that test is supposed to be sync.

Also there’s the question of whether you want the test to pass or not for the old < 12.0.0 implementation, where the semantics of specifiers were very different; so ESM that could be imported in Node 11 might not work in 12, and vice versa.

There’s always the option of just testing for the Node version number. Not exactly a feature detection, I know, but it would work and be sync.

[ljharb]

Perhaps you'd make a folder, and put a package.json in it, with these contents:

{
  "main": "./false.js",
  "exports": { ".": "./true.js" }
}

and false.js has module.exports = false and true.js has module.exports = true.

Then, your feature test is require('./path/to/folder')?

[guybedford]

This is a great suggestion @coreyfarrell @nicolo-ribaudo. I've created an issue to track at nodejs/node#30599. Ideally we can just combine the warning message and error into one now. See https://github.com/nodejs/node/blob/master/lib/internal/modules/cjs/loader.js#L1193 for where this is happening.

We were still not sure if we would be able to ship the error or if it would break tools. But if tools will genuinely benefit from this being a reliable error then certainly this sounds like a good pattern to me.

I don't have time right now to work on an implementation, but would gladly approve any PR and see that in a patch.

[bmeck]

.cjs appears to be supported in various tooling now, closing for now feel free to reopen