import-js / eslint-plugin-import

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

[Feature Request] Support new ESLint flat config #2556

Open TomerAberbach opened 1 year ago

TomerAberbach commented 1 year ago

ESLint has a new experimental config file format and this plugin doesn't work with it. The plugin fails at this line because the new config format doesn't pass parsers via paths. Instead the [parser object itself is passed](https://eslint.org/docs/latest/user-guide/configuring/configuration-files-new#:~:text=parser%20%2D%20Either%20an%20object%20containing%20a%20parse()%20method%20or%20a%20string%20indicating%20the%20name%20of%20a%20parser%20inside%20of%20a%20plugin%20(i.e.%2C%20%22pluginName/parserName%22).%20(default%3A%20%22%40/espree%22)).

I was able to mostly fix this in my fork, but because the parserPath doesn't exist anymore for this config format some of the keysFromParser logic won't work anymore.

Anyway, figured I'd open this issue to start discussion on the new config file format!

ljharb commented 1 year ago

If there's a way to support both configs at the same time, a PR would be appreciated.

Look to https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/pull/891 and https://github.com/jsx-eslint/eslint-plugin-react/pull/3429 for some preferred direction.

TomerAberbach commented 1 year ago

Yup, my fork does support both configs. The only thing I'm unsure about in my fork is how to adapt the keysFromParser logic to work without knowing the paths.

But based on this comment though, it sounds like maybe it's fine for the new config format not to set visitorKeys to anything? Let me know!

ljharb commented 1 year ago

As long as the rules and configs work in both config systems, we're good.

b0o commented 1 year ago

The plugin fails at this line because the new config format doesn't pass parsers via paths.

We're experiencing this issue as well. @TomerAberbach it would be really cool if you could submit a PR with your fixes!

TomerAberbach commented 1 year ago

Thanks for the message! Been a bit busy with other stuff and kind of forgot about this 😅

I'll try to pick this up soon!

goosewobbler commented 1 year ago

As long as the rules and configs work in both config systems, we're good.

I'll test this on my fork of @TomerAberbach's fork when I get a chance.

TomerAberbach commented 1 year ago

Thanks @goosewobbler! A little while ago I tried making it so that all the tests get run twice (once with the legacy and once with the new config system), but got stuck trying to get stuff working and then got busy 🙃

Would be awesome if you're able to figure out how to test both systems! Happy to answer any questions about the fork if that would help as well

CallMeLaNN commented 1 year ago

I managed to make it work without changes in parser.js source. Since parserPath can be parser name and keysFromParser can accept it, we can use the settings.import/parsers to add the missing parser from getParserPath function.

So that can pass the parserPath is required! error but actually I don't see parserPath is useful to keysFromParser because on my env it always doesn't hit the line mentioned in keysFromParser logic won't work anymore. So "espree" below is the default I guess from the source.

This is my working test config on node and typescript env. (EDIT: I just remove from using FlatCompat and directly construct the config)

import importPlugin from "eslint-plugin-import";

export default [
  // ... "eslint:recommended"
  {
    // All eslint-plugin-import config is here
    languageOptions: {
      parserOptions: {
        // Eslint doesn't supply ecmaVersion in `parser.js` `context.parserOptions`
        // This is required to avoid ecmaVersion < 2015 error or 'import' / 'export' error
        ecmaVersion: "latest",
        sourceType: "module",
      },
    },
    plugins: { import: importPlugin },
    settings: {
      // This will do the trick
      "import/parsers": {
        espree: [".js", ".cjs", ".mjs", ".jsx"],
      },
      "import/resolver": {
        typescript: true,
        node: true,
      },
    },
    rules: {
      ...importPlugin.configs["recommended"].rules,
    },
  },
  // ... add other plugins like typescript-eslint etc
  {
    // All my custom config
    languageOptions: {
      // This default get replaced by plugins, so I added back. Not related probably.
      ecmaVersion: "latest",
      sourceType: "module",
     // ... globals etc
    },
  },
  // ... custom rules etc
]

It was discovered and tested on a module js file with import/namespace rule throw like this. I lint the eslint.config.js itself lol

Parse errors in imported module '@eslint/eslintrc': parserPath is required! (undefined:undefined) eslint(import/namespace)
ziimakc commented 1 year ago

Any way to make it work in meantime with flat config?

UPD. espree did the trick:

"import/parsers": {
    espree: [".js", ".cjs", ".mjs", ".jsx"],
    "@typescript-eslint/parser": [".ts"],
},
500-internal-server-error commented 1 year ago

Hello, I'm still having problems with mixing this plugin with my ESLint flat config. The fork mentioned in the OP seems to be a bit old. Is there any way I can use this plugin with the new flat config?

$ npx eslint src/index.ts

Oops! Something went wrong! :(

ESLint: 8.42.0

Configuration for rule "plugins" is invalid. Expected severity of "off", 0, "warn", 1, "error", or 2.

You passed '"import"'.

See https://eslint.org/docs/latest/use/configure/rules#using-configuration-files for configuring rules.

Thanks in advance.

ljharb commented 1 year ago

Not until that support is released.

OlivierZal commented 11 months ago

Hi @ljharb and @timmywil,

I guess my issue is related to this one but I'm not sure: I've recently migrated my .eslintrc to the new eslint.config.js eslint flat config:

import js from '@eslint/js'
/* eslint-disable import/no-unresolved */
import tsParser from '@typescript-eslint/parser'
import tsPlugin from '@typescript-eslint/eslint-plugin'
/* eslint-enable import/no-unresolved */
import prettierConfig from 'eslint-config-prettier'
import importPlugin from 'eslint-plugin-import'
import globals from 'globals'

export default [
  {
    ignores: ['.homeybuild/'],
  },
  js.configs.recommended,
  {
    languageOptions: {
      ecmaVersion: 'latest',
      globals: {
        ...globals.browser,
        ...globals.es2024,
        ...globals.node,
      },
      sourceType: 'module',
    },
    linterOptions: {
      reportUnusedDisableDirectives: true,
    },
    plugins: {
      import: importPlugin,
    },
    rules: importPlugin.configs.recommended.rules,
  },
  {
    files: ['**/*.ts', '**/*.tsx', '**/*.mts', '**/*.cts'],
    languageOptions: {
      parser: tsParser,
      parserOptions: {
        project: './tsconfig.json',
      },
    },
    plugins: {
      '@typescript-eslint': tsPlugin,
    },
    rules: {
      ...tsPlugin.configs['eslint-recommended'].overrides[0].rules,
      ...tsPlugin.configs['strict-type-checked'].rules,
      ...tsPlugin.configs['stylistic-type-checked'].rules,
      ...importPlugin.configs.typescript.rules,
      '@typescript-eslint/no-unsafe-argument': 'off',
      '@typescript-eslint/no-unsafe-assignment': 'off',
      '@typescript-eslint/no-unsafe-call': 'off',
      '@typescript-eslint/no-unsafe-member-access': 'off',
      '@typescript-eslint/no-unsafe-return': 'off',
      '@typescript-eslint/no-unused-vars': [
        'error',
        {
          varsIgnorePattern: 'onHomeyReady',
        },
      ],
    },
    settings: {
      ...importPlugin.configs.typescript.settings,
      'import/resolver': {
        ...importPlugin.configs.typescript.settings['import/resolver'],
        typescript: {
          alwaysTryTypes: true,
        },
      },
    },
  },
  prettierConfig,
]

and I got the following errors:

  2:19  error    Parse errors in imported module 'axios': parserPath or languageOptions.parser is required! (undefined:undefined)  import/namespace
  2:19  error    Parse errors in imported module 'axios': parserPath or languageOptions.parser is required! (undefined:undefined)  import/default
  2:19  warning  Parse errors in imported module 'axios': parserPath or languageOptions.parser is required! (undefined:undefined)  import/no-named-as-default
  2:19  warning  Parse errors in imported module 'axios': parserPath or languageOptions.parser is required! (undefined:undefined)  import/no-named-as-default-member

Should it be solved with https://github.com/import-js/eslint-plugin-import/pull/2829, or is it another issue?

I'm also wondering why I'm getting this error (only for @typescript-eslint/* imports):

  2:22  error  Unable to resolve path to module '@typescript-eslint/parser'         import/no-unresolved
  3:22  error  Unable to resolve path to module '@typescript-eslint/eslint-plugin'  import/no-unresolved
ljharb commented 11 months ago

@OlivierZal yes, i suspect #2829 will solve your issue.

For the other one, are those packages installed on disk and in package.json?

OlivierZal commented 11 months ago

Thanks @ljharb for your answer.

Yes, they are installed and work very well: @typescript-eslint errors are correctly raised when the linter is run.

So do you think it is worth raising a separate issue on this one?

ljharb commented 11 months ago

Yes, I think so, thanks

simlu commented 11 months ago

Running into the same error here parserPath or languageOptions.parser is required! when trying to use FlatESLint.

I'll keep an eye on #2829 for sure :+1:

trivikr commented 3 months ago

I visited PRs linked with this issue, and noticed that many users are temporarily disabling eslint-plugin-import rules when upgrading to ESLint v9.

What's the latest updated on compatibility with ESLint v9?

ljharb commented 3 months ago

@trivikr see #2948. One simply shouldn't upgrade to v9 until all of the configs and plugins one uses support it, just like every other time eslint has bumped the major.

JavaScriptBach commented 2 months ago

Has anyone managed to get this plugin to work on a TypeScript project using FlatCompat?

ljharb commented 2 months ago

@JavaScriptBach in theory, all of our rules except one should work with FlatCompat, but one of them can't work yet because flat config lacks the capacity to support it.

arslivinski commented 2 months ago

@ljharb which one?

ljharb commented 2 months ago

@arslivinski no-unused-modules

arslivinski commented 2 months ago

Every time that I enable the no-unused-modules rule, I end up disabling it not long after because it always breaks my flow when splitting a large PR into smaller ones, as I always end with unused modules. I understand that it is MY use case and doesn't reflect how this plugin is used at large.

That being said, supporting flat configs and ESLint >= 9 could be considered a breaking change? If so, couldn't we also remove this rule if it is the only reason holding this back?

Or, as another alternative, would be too much effort to have two exports? The default one using the previous config engine and an alternative one using the flat config engine? I think that this could be possible if we target a minimum version of ESLint 8 that has support for the new flat config, and to use the flat config we have to explicitly import the plugin. I would be pleased to submit a PR for this, if it is something that the maintainers see as a possible solution.

guillaumebrunerie commented 2 months ago

Every time that I enable the no-unused-modules rule, I end up disabling it not long after because it always breaks my flow when splitting a large PR into smaller ones, as I always end with unused modules. I understand that it is MY use case and doesn't reflect how this plugin is used at large.

Interesting, I have the opposite experience. Personally I find no-unused-modules to be by far the most useful rule this plugin offers, as it allows finding dead code that would be very hard to find otherwise.

However, I've also found that it often doesn't work very well in my editor, it regularly reports some exports as unused even though "find all references" finds several references in other files. Linting from the command line works fine.

I wonder if there could be a compromise consisting of activating this rule only when linting the whole project and not when linting a specific file? From what I understand, requiring access to other files than the one being linted is what causes problems in ESLint 9, so maybe that would be easier?

ljharb commented 2 months ago

@arslivinski no, it needn't be a breaking change, it will be semver-minor. but regardless, i wouldn't want to claim flat config support and not have 100% of it.

arslivinski commented 2 months ago

@ljharb completely understand your point of view. However, do you know if there's some change planned for ESLint that would make this rule viable again? If not, what's the plan, just wait? I really want to contribute. 😄

Off-topic > > Every time that I enable the `no-unused-modules` rule, I end up disabling it not long after because it always breaks my flow when splitting a large PR into smaller ones, as I always end with unused modules. I understand that it is **MY** use case and doesn't reflect how this plugin is used at large. > > Interesting, I have the opposite experience. Personally I find `no-unused-modules` to be by far the most useful rule this plugin offers, as it allows finding dead code that would be very hard to find otherwise. @guillaumebrunerie in the past, when I was in need to do a spring cleanup on codebase, I always used [Madge](https://www.npmjs.com/package/madge) for the task of finding unused modules. My conclusion is that this method of doing a cleanup occasionally is more effective (and faster!) than running `no-unused-modules`. > However, I've also found that it often doesn't work very well in my editor, it regularly reports some exports as unused even though "find all references" finds several references in other files. Linting from the command line works fine. > > I wonder if there could be a compromise consisting of activating this rule only when linting the whole project and not when linting a specific file? From what I understand, requiring access to other files than the one being linted is what causes problems in ESLint 9, so maybe that would be easier? You can disable the rule on the default config and enable it only when running on the CI or a pre-commit/push hook.
ljharb commented 2 months ago

@arslivinski yes, if we can affirm to the eslint team that their proposed solution will work for us, they'll ship something in v9 we can use (and polyfill for v8 and earlier v9s) moving forward. The work hasn't been done to verify that yet, though.

glarget commented 2 months ago

Hi, I tried to migrate to eslint v9 and understand that eslint-plugin-import is still in WIP to support it.

I have a question related to "import/order" rules.

Does this rule supposed to work with the flat config ?

I tried it but the format on save with doesn't work as expected.

I guess that I have to wait until migrating to V9 or maybe I do something wrong ?

Here is a code sample. Thank you very much for your help !

import js from '@eslint/js';
import importPlugin from 'eslint-plugin-import';
import eslintPluginPrettierRecommended from 'eslint-plugin-prettier/recommended';

export default [
  js.configs.recommended,
  ...tseslint.configs.recommended,
 plugins: {
      react,
      'react-hooks': reactHooks,
      import: importPlugin,
  },
rules: { 
  'import/order': [
        'error',
        {
          groups: ['builtin', 'external', 'internal', 'parent', 'sibling', 'index', 'object'],
          'newlines-between': 'always',
          alphabetize: {
            order: 'asc',
            caseInsensitive: true,
          },
          pathGroups: [
            {
              pattern: 'react',
              group: 'external',
              position: 'before',
            },
          ],
          pathGroupsExcludedImportTypes: ['react'],
          warnOnUnassignedImports: true,
        },
      ],
  },
  eslintPluginPrettierRecommended
]
ljharb commented 2 months ago

@glarget no, this plugin does not yet work with flat config or with v9.

lgenzelis commented 2 months ago

For anyone else landing here looking for a solution, here's my eslint.config.cjs, which works for me :)

const pluginImport = require('eslint-plugin-import');

module.exports = [
  {
    plugins: {
      import: { rules: pluginImport.rules },
    },
    rules: {
      'import/order': 'error',
      'import/group-exports': 'error',
      'import/exports-last': 'error',
    },
  },
  // ... all your other other configurations
];
csvan commented 2 months ago

@lgenzelis are you sure all the rules actually work in the above? The fact that you can successfully run eslint with the above config is not an indication that the plugin actually works, it just means you get a lot of false positives.

OlivierZal commented 2 months ago

On my end, the following flat config (extract) worked with ESlint 8 but raises an error with ESlint 9:

    plugins: {
      import: importPlugin,
    },
    rules: {
      ...importPlugin.configs.recommended.rules, // flat config does not support the old `plugins` format
      <custom rules>
    }

Error raised:

TypeError: context.getAncestors is not a function
Occurred while linting /Users/.../src/index.ts:66
Rule: "import/no-named-as-default"
    at importDeclaration (/Users/.../node_modules/eslint-plugin-import/lib/importDeclaration.js:2:27)
    at checkDefault (/Users/.../node_modules/eslint-plugin-import/lib/rules/no-named-as-default.js:21:62)
    at ruleErrorHandler (/Users/.../node_modules/eslint/lib/linter/linter.js:1115:48)
    at /Users/.../node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/Users/.../node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/Users/.../node_modules/eslint/lib/linter/node-event-generator.js:297:26)
    at NodeEventGenerator.applySelectors (/Users/.../node_modules/eslint/lib/linter/node-event-generator.js:326:22)
    at NodeEventGenerator.enterNode (/Users/.../node_modules/eslint/lib/linter/node-event-generator.js:340:14)
    at runRules (/Users/.../node_modules/eslint/lib/linter/linter.js:1154:40)

Not all the import plugin rules fail, but some of the recommended config do.

I think that such a compatibility issue with ESlint 9 needs to be addressed before to think about flat config.

ljharb commented 2 months ago

@OlivierZal flat config works on eslint 8, so that's what i'd want to support first.

lgenzelis commented 1 month ago

@lgenzelis are you sure all the rules actually work in the above? The fact that you can successfully run eslint with the above config is not an indication that the plugin actually works, it just means you get a lot of false positives.

@csvan not saying that every rule works, but those specific 3 do (I validated it). Oh, and I've should've mentioned: I'm using eslint 8, not 9.

trivikr commented 1 month ago

I noticed that some projects are moving to https://www.npmjs.com/eslint-plugin-import-x Since it's a fork of eslint-plugin-import, maybe it can be considered as a workaround.

Ref: https://twitter.com/karlhorky/status/1792500813320864023

trivikr commented 1 month ago

I just discovered that mentions of forks which fix ESLint v9 upgrade blockers, like one in previous comment, are being marked as spam.

I don't want to digress, but some discussion has happened about this at https://github.com/import-js/eslint-plugin-import/issues/2948#issuecomment-2119652271

ljharb commented 1 month ago

@trivikr yes, advertising a replacement project is always spam. There's nothing to discuss.

Users of this plugin should simply not upgrade to eslint 9 yet.

IanVS commented 1 month ago

Note that context.parserOptions is deprecated, and so some rules like no-default-export which rely on them can break with flat configs, especially when using typescript-eslint 7.2.0+ (after https://github.com/typescript-eslint/typescript-eslint/pull/8622).

Mustachipleb commented 1 month ago

Is there still no official workaround for this?

ljharb commented 1 month ago

@Mustachipleb nope, that's why the issue is still open.

trivikr commented 1 month ago

Is there still no official workaround for this?

There are some unofficial workarounds listed in the previous comments, like https://github.com/import-js/eslint-plugin-import/issues/2556#issuecomment-2123669485, but they were marked as spam or off-topic.

olegafx commented 1 month ago

@Mustachipleb nope, that's why the issue is still open.

Even with FlatCompat from @eslint/eslintrc?

Mustachipleb commented 1 month ago

@Mustachipleb nope, that's why the issue is still open.

I'll rephrase. If I understand correctly, some rules use functionality eslint no longer provides with flat config. If after 2 years, and certainly now that flat config is becoming the default, there is still no fix in sight; would it not be acceptable to create a patched version under a non-latest tag that disables the offending rules?

As it stands this library is effectively in limbo until this is addressed, at least that's how I see it.

ljharb commented 1 month ago

@Mustachipleb it's not up to me whether it's acceptable. i just wouldn't want to allow an unofficial solution to be spammed here.

Yes, you're right, the library is in limbo until it's addressed.

michaelfaith commented 1 month ago

Just catching up on this, it seems like the main blocking issue is that the legacy config uses a parser by name, whereas the flat config needs the parser object? And in the typescript config we're just setting the parser by name. What if, in a new flat config version of the typescript config we setting the parser as the typescript parser object, and added the @typescript-eslint/parser package as an optional peer dependency?

ljharb commented 1 month ago

@michaelfaith adding an optional peer dependency is still a breaking change, both because it constrains the version range of one that might already be installed, and because not every npm version we support, supports optional peer deps.

Goldziher commented 3 weeks ago

Hi there, if this is impossible, should this plugin be forked?

goosewobbler commented 3 weeks ago

@Goldziher Discussing forks is seen as spam here but there is a prominent one mentioned above. However they also have an open issue for flat config so YMMV.

Goldziher commented 3 weeks ago

@Goldziher Discussing forks is seen as spam here but there is a prominent one mentioned above. However they also have an open issue for flat config so YMMV.

I see. This is no spam. This plugin is important and the fact this issue is two years old is problematic.

Maintaining this plugin is a major effort, I do not doubt this. Hence the question - if it's impossible to move forward in this package without breaking existing dependencies, this might well necessitate a well maintained fork.

ljharb commented 3 weeks ago

@Goldziher its not impossible, just difficult.