Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for dynamic import() to be used in eslint config file #13684

Closed
KilianKilmister opened this issue Sep 11, 2020 · 9 comments
Closed

Allow for dynamic import() to be used in eslint config file #13684

KilianKilmister opened this issue Sep 11, 2020 · 9 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion auto closed The bot closed this issue core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion

Comments

@KilianKilmister
Copy link

KilianKilmister commented Sep 11, 2020

The version of ESLint you are using.

ESLint-v7.8.1

The problem you want to solve.

Currently, while eslint understands the meaning of the .cjs extension, it is unable to load a config file that uses dinamic import() statements. It fails with an error:

TypeError [ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING]: A dynamic import callback was not specified.

This is caused by the dependency v8-compile-cache which patches the node internal Module.prototype._compile method in an insufficient fashion.

The purpouse of solving this is easier handeling of eslint configs for esm-users until config-file-simplification arrives (RFC). The Roadmap estimated the implementation to arrive in 5 months (6 months when the statement was made).

Your take on the correct solution to problem.

As the problem is caused by a dependency, there is no straight forward solution. I'll file an issue in in the project after i'm done here. issue filed

NOTE: While debugging the issue, I found some things that are potential road-blocks for esm support (like the dependency import-fresh, which dispite the name is for loading cjs). I'll be doing some more exploring and will post a proper report (propaby in the RFC repo) once i have some spare time.

Are you willing to submit a pull request to implement this change?

Potentially. I'll check what exactly would need to have to change in v8-compile-cache and possibly make a PR there to solve the problem at the root. Besides that, it depends on what I find while examining the things mentioned in the note.

NOTE: A quick-fix is setting the env-variable DISABLE_V8_COMPILE_CACHE

@KilianKilmister KilianKilmister added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint triage An ESLint team member will look at this issue soon labels Sep 11, 2020
@mdjermanovic mdjermanovic added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Sep 12, 2020
@mdjermanovic
Copy link
Member

Hi @KilianKilmister, thanks for the issue!

Could you please share an example of a config file with dynamic imports?

Does it work when you set the env-variable DISABLE_V8_COMPILE_CACHE?

@KilianKilmister
Copy link
Author

i've had issues properly reproducing it with a simpler loader. this is a slight modification of the one i used originally. it was supposed to load a merge a directory of sub-configs.

Trying to load this normaly does throw TypeError [ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING]: A dynamic import callback was not specified. again.

Using DISABLE_V8_COMPILE_CACHE does resolve the error, altough it looks like the config isn't properly loaded. probably because of the async nature, but that's not too important.

I have very little experience with CommonJs, so i can't make much of a statement around why the loading isn't working this way.

From the issue field in v8-compile-cache:

The error originates from an insufficient options object passed to the new Script()-constructor node VM docs

It's using the nodejs vm module and the older Script class was designed before the ESM standard and the newer Module class is still behind an experimental flag, so it's a bit complicated

// .eslintrc.cjs
(async () => {
  const entries = []
   entries.push(await import('./test.js'))

  module.exports = {
    ...entries.map(dissector).reduce(reducer, {}),
    root: true
  }
})()


function dissector (item, index) {
  return Object.entries(item)
}
/**
 *
 * @param {{[key: string]: unknown}} prev
 * @param {[string, any]} curr
 */
function reducer (prev, [key, value]) {
  if (!(key in prev)) prev[key] = value
  else prev[key] = {
    ...prev[key], ...value
  }
  return prev
}
// test.js
export default {
  rules: {
    camelcase: 'error'
  }
}

@mdjermanovic
Copy link
Member

// .eslintrc.cjs
(async () => {
  const entries = []
   entries.push(await import('./test.js'))

  module.exports = {
    ...entries.map(dissector).reduce(reducer, {}),
    root: true
  }
})()

This won't work as expected. Requiring .eslintrc.cjs will return the default empty object for module.exports.

For example:

//----- a.js -----
(async () => {
  module.exports = await { foo: "bar" };
})();
//----- b.js -----
console.log(require("./a")); // logs {}

The assignment to module.exports happens later in the queue:

//----- b.js -----
console.log(require("./a")); // logs {}
setTimeout(() => console.log(require("./a"))); // logs { foo: 'bar' }

Unfortunately, I don't think it's possible to have an async config with the current eslint config system, so I believe there's no way to use dynamic imports in eslint config files at the moment, regardless of the v8-compile-cache issues.

@KilianKilmister
Copy link
Author

@mdjermanovic thanks for the explanation. I somewhat expected this not to work. But i wasn't sure as i avoid CommonJS whenever it's possible.

Should this be kept open for the v8-compile-cache and co. stuff?

@mdjermanovic
Copy link
Member

This isn't a problem at the moment, since it doesn't seem possible to use import() in actual config files anyway.

But, v8-compile-cache throwing on import() might be a problem for the new config system if we use import() to load configs. Also, the new config system will support async configs, which should be, then, able to use import().

@nzakas this issue might be relevant to #13481.

@nzakas
Copy link
Member

nzakas commented Sep 19, 2020

I’ll keep an eye out as I go. Node.js ESM support is still considered experimental, so definitely worth following up with them about this issue.

@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label Oct 20, 2020
@eslint-deprecated
Copy link

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@trusktr
Copy link

trusktr commented Nov 6, 2020

The assignment to module.exports happens later in the queue:

it doesn't seem possible to use import() in actual config files anyway.

Here's how to import ES Modules in a synchronous way:

const deasync = require('deasync'); // powerful magic

const promiseSync = deasync((promise, cb) => {
  promise.then((result) => cb(null, result)).catch((error) => cb(error));
});

const importSync = (name) => promiseSync(import(name));

// Look, no await needed here!
const something = importSync('./something.js').default;

module.exports = {
  ...something,
  // ...
}

See deasync

Running with DISABLE_V8_COMPILE_CACHE=1 eslint . works great!

@mdjermanovic
Copy link
Member

Awesome, thanks for sharing this!

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Apr 19, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Apr 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion auto closed The bot closed this issue core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion
Projects
None yet
Development

No branches or pull requests

4 participants