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

fix(linter)!: override plugins array when passed in config file #7303

Conversation

camchenry
Copy link
Contributor

@camchenry camchenry commented Nov 15, 2024

When the plugins config property is specified, it will overwrite the default plugins array. This allows the plugins list to be easily customized and allows for disabling default plugins at the same time as enabling non-default ones.

  • example: { "plugins": [] } will enable no plugins.
  • example: { } will enable default plugins.
  • example: { "plugins": ["typescript", "import"] } will enable only the import and typescript plugins.

Copy link

graphite-app bot commented Nov 15, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions bot added A-linter Area - Linter C-bug Category - Bug labels Nov 15, 2024
Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@camchenry camchenry changed the title fix(linter): override plugins array when passed in config file fix(linter)!: override plugins array when passed in config file Nov 15, 2024
Copy link

codspeed-hq bot commented Nov 15, 2024

CodSpeed Performance Report

Merging #7303 will create unknown performance changes

Comparing 11-15-fix_linter_override_plugins_array_when_passed_in_config_file (20d9080) with main (d135d3e)

Summary

🆕 30 new benchmarks

Benchmarks breakdown

Benchmark main 11-15-fix_linter_override_plugins_array_when_passed_in_config_file Change
🆕 codegen[checker.ts] N/A 24.6 ms N/A
🆕 codegen_sourcemap[checker.ts] N/A 77.4 ms N/A
🆕 isolated-declarations[vue-id.ts] N/A 397.5 ms N/A
🆕 lexer[RadixUIAdoptionSection.jsx] N/A 24 µs N/A
🆕 lexer[antd.js] N/A 22.3 ms N/A
🆕 lexer[cal.com.tsx] N/A 5.5 ms N/A
🆕 lexer[checker.ts] N/A 13.3 ms N/A
🆕 lexer[pdf.mjs] N/A 3.6 ms N/A
🆕 linter[RadixUIAdoptionSection.jsx] N/A 2.4 ms N/A
🆕 linter[cal.com.tsx] N/A 1.1 s N/A
🆕 linter[checker.ts] N/A 2.5 s N/A
🆕 minifier[antd.js] N/A 437.2 ms N/A
🆕 minifier[react.development.js] N/A 2.9 ms N/A
🆕 minifier[typescript.js] N/A 545.4 ms N/A
🆕 parser[RadixUIAdoptionSection.jsx] N/A 77.7 µs N/A
🆕 parser[antd.js] N/A 106.5 ms N/A
🆕 parser[cal.com.tsx] N/A 24.6 ms N/A
🆕 parser[checker.ts] N/A 53.5 ms N/A
🆕 parser[pdf.mjs] N/A 17.4 ms N/A
🆕 semantic[RadixUIAdoptionSection.jsx] N/A 97.8 µs N/A
... ... ... ... ...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

@camchenry camchenry force-pushed the 11-15-fix_linter_override_plugins_array_when_passed_in_config_file branch from 226dba0 to be13ed4 Compare November 15, 2024 20:10
@camchenry camchenry marked this pull request as ready for review November 15, 2024 20:26
@Sysix
Copy link
Collaborator

Sysix commented Nov 15, 2024

Thanks for working on this. ❤️ With the new overrides property there is a new discussion to make :)

{
  plugins: ["typescript"],
  overrides: [
    {
        files: ["*.tsx"],
        plugins: ["react"]
    }
  ]
}

with this config I expect to enable ts and react plugin for .tsx files. Maybe someone other expect it differently :)

@camc314
Copy link
Contributor

camc314 commented Nov 15, 2024

if i specify an empty array. but have a rule in the rules list that uses a plugin does it get reported as an unknown rule?

@Sysix
Copy link
Collaborator

Sysix commented Nov 15, 2024

That is a also a topic 👍 Let not block this PR, perrsonally im fine at the moment.

if i specify an empty array. but have a rule in the rules list that uses a plugin does it get reported as an unknown rule?

existing rules in the project should always be known and can be enabled/disabled in the rules block.
plugins + categories enables the base rules, rules block should adjust the last needs.

That what I expect as a user, don't know what the core team says about it :)

@camchenry
Copy link
Contributor Author

Thanks for working on this. ❤️ With the new overrides property there is a new discussion to make :)

{
  plugins: ["typescript"],
  overrides: [
    {
        files: ["*.tsx"],
        plugins: ["react"]
    }
  ]
}

with this config I expect to enable ts and react plugin for .tsx files. Maybe someone other expect it differently :)

Good point, I forgot to consider the overrides property here. I think that making the plugins array append when inside of overrides feels the most intuitive to me? I can update the tests and behavior accordingly.

if i specify an empty array. but have a rule in the rules list that uses a plugin does it get reported as an unknown rule?

Yes, I think this should report as an unknown/disabled rule warning. I can imagine a case where you remove a plugin that is no longer needed and want to be warned of config that is no longer useful. Or, like in the case above with overrides, you need to explicitly enable the plugin to use the rule.

@Sysix
Copy link
Collaborator

Sysix commented Nov 15, 2024

I think that making the plugins array append when inside of overrides feels the most intuitive to me?

Feels for me too, but I do not know how it is currently with categories.
When overrides really overrides and not append. I think my expection is wrong and we should keep it consistent.

@camchenry
Copy link
Contributor Author

I checked the behavior with the legacy .eslintrc setup in ESLint, and it this config will append so that .ts files have the react and typescript plugins enabled:

{
  "plugins": ["@typescript-eslint"],
  "overrides": [
    {
      "files": ["*.ts", "*.tsx"],
      "plugins": ["react"]
    }
  ]
}

So, I think this is the way to go for compatibility. And specifying an empty array here does not disable all of the plugins.

@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Nov 16, 2024
Copy link
Member

Boshen commented Nov 16, 2024

Merge activity

  • Nov 16, 12:41 AM EST: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Nov 16, 12:41 AM EST: A user added this pull request to the Graphite merge queue.
  • Nov 16, 1:00 AM EST: A user merged this pull request with the Graphite merge queue.

- fixes #6896

When the `plugins` config property is specified, it will overwrite the default plugins array. This allows the plugins list to be easily customized and allows for disabling default plugins at the same time as enabling non-default ones.

- example: `{ "plugins": [] }` will enable no plugins.
- example: `{ }` will enable default plugins.
- example: `{ "plugins": ["typescript", "import"] }` will enable only the import and typescript plugins.
@Boshen Boshen force-pushed the 11-15-fix_linter_override_plugins_array_when_passed_in_config_file branch from be13ed4 to 20d9080 Compare November 16, 2024 05:54
@graphite-app graphite-app bot merged commit 20d9080 into main Nov 16, 2024
26 checks passed
@graphite-app graphite-app bot deleted the 11-15-fix_linter_override_plugins_array_when_passed_in_config_file branch November 16, 2024 06:00
Dunqing pushed a commit that referenced this pull request Nov 17, 2024
- fixes #6896

When the `plugins` config property is specified, it will overwrite the default plugins array. This allows the plugins list to be easily customized and allows for disabling default plugins at the same time as enabling non-default ones.

- example: `{ "plugins": [] }` will enable no plugins.
- example: `{ }` will enable default plugins.
- example: `{ "plugins": ["typescript", "import"] }` will enable only the import and typescript plugins.
Dunqing pushed a commit that referenced this pull request Nov 18, 2024
- fixes #6896

When the `plugins` config property is specified, it will overwrite the default plugins array. This allows the plugins list to be easily customized and allows for disabling default plugins at the same time as enabling non-default ones.

- example: `{ "plugins": [] }` will enable no plugins.
- example: `{ }` will enable default plugins.
- example: `{ "plugins": ["typescript", "import"] }` will enable only the import and typescript plugins.
Dunqing pushed a commit that referenced this pull request Nov 18, 2024
- fixes #6896

When the `plugins` config property is specified, it will overwrite the default plugins array. This allows the plugins list to be easily customized and allows for disabling default plugins at the same time as enabling non-default ones.

- example: `{ "plugins": [] }` will enable no plugins.
- example: `{ }` will enable default plugins.
- example: `{ "plugins": ["typescript", "import"] }` will enable only the import and typescript plugins.
Boshen added a commit that referenced this pull request Nov 20, 2024
## [0.12.0] - 2024-11-20

- 20d9080 linter: [**BREAKING**] Override plugins array when passed in
config file (#7303) (camchenry)

### Features

- 1d9f528 linter: Implement `unicorn/prefer-string-raw` lint rule
(#7335) (Ryan Walker)
- d445e0f linter: Implement `unicorn/consistent-existence-index-check`
(#7262) (Ryan Walker)
- 01ddf37 linter: Add `allowReject` option to
`no-useless-promise-resolve-reject` (#7274) (no-yan)
- 755a31b linter: Support bind function case for compatibility with
`promise/no-return-wrap` (#7232) (no-yan)
- 428770e linter: Add `import/no-namespace` rule (#7229) (Dmitry
Zakharov)
- 9c91151 linter: Implement typescript/no-empty-object-type (#6977)
(Orenbek)
- 2268a0e linter: Support `overrides` config field (#6974) (DonIsaac)
- 3dcac1a linter: React/exhaustive-deps (#7151) (camc314)
- d3a0119 oxlint: Add `cwd` property to `LintRunner` (#7352) (Alexander
S.)

### Bug Fixes

- ba0b2ff editor: Reload workspace configuration after change (#7302)
(Alexander S.)
- bc0e72c linter: Handle user variables correctly for import/no_commonjs
(#7316) (Dmitry Zakharov)
- bf839c1 linter: False positive in `jest/expect-expect` (#7341)
(dalaoshu)
- ff2a1d4 linter: Move `exhaustive-deps` to `react` (#7251) (camc314)
- df5c535 linter: Revert unmatched rule error (#7257) (Cameron A
McHenry)
- c4ed230 linter: Fix false positive in eslint/no-cond-assign (#7241)
(camc314)
- ef847da linter: False positive in `jsx-a11y/iframe-has-title` (#7253)
(dalaoshu)
- 62b6327 linter: React/exhaustive-deps update span for unknown deps
diagnostic (#7249) (camc314)

### Documentation

- 4c124a8 editor/vscode: Update VS Code readme with installation
instructions and available features (#7306) (Nicholas Rayburn)

### Refactor

- c6a4868 linter: Temporarily remove unknown rules checking (#7260)
(camchenry)

### Testing

- 5190b7f editor: Add test setup (#7361) (Alexander S.)

Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0-merge Merge with Graphite Merge Queue A-linter Area - Linter C-bug Category - Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

linter: default plugins are always enabled in config file
4 participants