import-js / eslint-plugin-import

ESLint plugin with rules that help validate proper imports.
MIT License
5.53k stars 1.56k forks source link

v2.23 no longer works with `Promise`-based Webpack configuration #2069

Open eamodio opened 3 years ago

eamodio commented 3 years ago

Prior to v2.23, my webpack.config which returns a Promise<WebpackConfig[]> worked fine, but now it causes the following errors (LOTS of them):

Webpack config returns a `Promise`; that signature is not supported at the moment. Using empty object instead.
ljharb commented 3 years ago

How did it work fine? The webpack resolver never actually awaited the config, so it seems like it probably always used an empty object?

In other words, the warning is showing you that it never worked, it's not that it suddenly stopped working.

I do think we should cache the webpack config, which would avoid logging this warning more than once.

JounQin commented 2 years ago

This can be implemented with https://github.com/un-ts/synckit for now because ESLint does not support async rules.

ImanMahmoudinasab commented 2 years ago

@JounQin @ljharb I may get some time to work on this. Do you think the following changes will be the right direction?

./resolvers/webpack/index.js:

import { createSyncFn } from 'synckit'

// todo: line 86 should be replaced by the following three lines:
const syncImport = createSyncFn(require.resolve('./resolvers/webpack/syncImport.js'));
// Get webpack config synchronously
webpackConfig = syncImport(configPath);

// todo: lines 128 ~ 132 should be removed

./resolvers/webpack/syncImport.js:

import { runAsWorker } from 'synckit'

runAsWorker(async (configPath) => {
  let config = await import(configPath);
  if(config.then){
    config = await config;
  }
  return config;
})
ljharb commented 2 years ago

I’m very skeptical about using anything that artificially forces async things to be sync. That should be something you can use in your config, but not something that should be built into this plugin.

JounQin commented 2 years ago

The downside is synckit(or worker_thread actually) is not compatible with current node versions we're supporting here.

So an alternative would be you can write another 3rd-party resolver instead.

–––

if(config.then){
  config = await config;
}

These codes are unnecessary, await already handles it. And you need to consider .default exports.

–––

synckit@0.1.6 is using child_process, so it still could be used in official implementation.

But the challenge is the results must be sterilizable, so returning the config is meaningless, the worker should handle the resolver's logic.

JounQin commented 2 years ago

I’m very skeptical about using anything that artificially forces async things to be sync. That should be something you can use in your config, but not something that should be built into this plugin.

ESLint forces things to be sync, not the user . 😅

What means async webpack config does not work at all now.

But I think it should be done in a 3rd-party resolver.

ImanMahmoudinasab commented 2 years ago

That should be something you can use in your config, but not something that should be built into this plugin.

But I think it should be done in a 3rd-party resolver.

Honestly, none of these solutions looks clean and wise solution, IMO.

ljharb commented 2 years ago

@JounQin yes, but i'm saying that async code should never be artificially made synchronous; that's been a bad practice in node ever since https://www.npmjs.com/package/deasync was new, almost a decade ago.

Async webpack config has never worked for eslint, and can't ever work reliably until eslint allows async configuration.

JounQin commented 2 years ago

Async webpack config has never worked for eslint, and can't ever work reliably until eslint allows async configuration.

I'm totally fine not to include such tools in the built-in, but a bit against with this conclusion.

The js ecosystem has been moving to ES module for a long time, but ESLint does not support pure ESM plugin, that makes the plugin authors can not upgrade pure ESM dependencies. For example, mdx@v2 ecosystem. eslint-mdx wants to support mdx@v2 ASAP, the limitation from ESLint is the worst part. That's why I developed synckit.

deasync is not a clever solution because it requires node bindings or node-gyp while sync-threads is out of maintainace for about two years.

My point is, if we want to support something we extremely want before ESLint, we do have solutions, And don't need to wait.

ljharb commented 2 years ago

The JS ecosystem has only very recently begun moving to ESM, and it's not even remotely close to 1% in terms of authors (considering "packages" skews the perspective). eslint obviously needs to start supporting ESM plugins, but it's eslint - and only eslint - that owns/should own the timeline for that feature.

I'm not clear on how synckit can actually work without C++ bindings, but I'm happy to hear more about it.

Separately, I don't see any value in supporting something like this before eslint does. Until eslint supports it, we simply don't need to.

JounQin commented 2 years ago

@ImanMahmoudinasab

Honestly, none of these solutions looks clean and wise solution, IMO.

Then you have to wait ESLint to support pure ESM and async rules, which could take a long time.

Personally I don't use webpack resolver, or even the Promise configuration. 😅

JounQin commented 2 years ago

I'm not clear on how synckit can actually work without C++ bindings, but I'm happy to hear more about it.

It is using worker-threads and Atomics.wait() under the hood, see also https://github.com/un-ts/synckit/blob/dba496db474d50415161e01c3bb38ccb18bc34ea/src/index.ts#L335

The idea came from sync-threads package and modern esbuild.

Separately, I don't see any value in supporting something like this before eslint does. Until eslint supports it, we simply don't need to.

I agree that the built-in webpack resolver does not need to support this usage, that's why I said it could be implemented in a 3rd-party resolver, if the user desires it strongly.

JounQin commented 2 years ago

@ljharb I'd like to create a eslint-import-resolver-webpack-async repository in this organization, HDYT?

ljharb commented 2 years ago

It'd probably be best to create it independently at first, and see it get some adoption, and then transfer it into the org?

JounQin commented 2 years ago

It'd probably be best to create it independently at first, and see it get some adoption, and then transfer it into the org?

Good idea.