-
-
Notifications
You must be signed in to change notification settings - Fork 722
fix(language_server): ignore JS plugins #14379
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
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
CodSpeed Performance ReportMerging #14379 will not alter performanceComparing Summary
Footnotes
|
8118f30 to
fa3712d
Compare
f05c765 to
4f75921
Compare
4f75921 to
bfec327
Compare
bfec327 to
5bcc4d6
Compare
5bcc4d6 to
6e239d3
Compare
oxc/crates/oxc_language_server/src/linter/server_linter.rs Lines 112 to 118 in d36d227
we |
Ah ha! Thanks. Have altered the test fixture to account for this. The config now produces different output from default, so now if the config is ignored, the test fails. This test now does fail on main, and passes on this PR. So this PR is necessary to produce correct behavior in language server when project uses JS plugins. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue where the language server would error when encountering JS plugins in configuration files. Previously, the presence of JS plugins would cause the language server to fail, even though they aren't supported in that context.
- Added an
is_enabledfield toExternalPluginStoreto control plugin loading behavior - Updated documentation to clarify JS plugin limitations in language server
- Modified configuration loading to silently ignore JS plugins when they're disabled
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/oxc_linter/src/external_plugin_store.rs | Added is_enabled field and constructor to control plugin functionality |
| crates/oxc_linter/src/config/config_builder.rs | Modified to skip JS plugin loading when disabled and updated error message |
| crates/oxc_linter/src/config/rules.rs | Added conditional logic to ignore JS plugin rules when plugins are disabled |
| crates/oxc_language_server/src/linter/server_linter.rs | Updated to create ExternalPluginStore with plugins disabled and added test |
| crates/oxc_linter/src/config/oxlintrc.rs | Added documentation about JS plugin limitations |
| crates/oxc_linter/src/config/overrides.rs | Added documentation about JS plugin limitations |
| npm/oxlint/configuration_schema.json | Updated descriptions to include JS plugin limitations |
| crates/oxc_linter/src/snapshots/schema_json.snap | Updated schema snapshot with new descriptions |
| tasks/website/src/linter/snapshots/schema_markdown.snap | Updated documentation snapshot |
| crates/oxc_language_server/fixtures/linter/js_plugins/* | Added test fixtures for JS plugin scenario |
| crates/oxc_language_server/src/snapshots/* | Added test snapshot for JS plugin handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Merge activity
|
Previously we produced an error when JS plugins are defined in config but no `ExternalLinter` is present. That's the desired behavior in `oxlint` CLI, because it happens when user is running Oxlint on an unsupported platform (32-bit, or big-endian). But it's *not* what we want to happen in language server. Language server does not yet support JS plugins, but user may want to use them in their project (via `oxlint` CLI), and language server should not fail with a "JS plugins are not supported" error on any file in such a project. This PR prevents that by adding an `is_enabled` field to `ExternalPluginStore`. By default, `is_enabled = true`, but language server sets it to `false`. When `false`, JS plugins are silently ignored, rather than producing an error. Note: A better solution would be to pass around `Option<ExternalPluginStore>` instead of `ExternalPluginStore` having an `is_enabled` flag. But JS plugin support will be added to the language server pretty soon, at which point we'll need to revert this change. `is_enabled` flag approach will produce less churn when we do that.
f75d0d5 to
6e8d2f6
Compare
## [1.21.0] - 2025-10-08 ### 🚀 Features - 576be20 linter/plugins: Support selectors DSL (#14435) (overlookmotel) - b2de44f linter/plugins: Support interpolation in normal diagnostic `message` (#14419) (overlookmotel) - 382c5be linter/plugins: Support placeholders in messageIds (#14416) (camc314) - 529e88e linter/plugins: Support `messageId`s (#14415) (camc314) - 83e7824 linter: Add `vue/require-default-export` rule (#14351) (Sysix) - ff98536 linter: Add `vue/no-import-compiler-macros` rule (#14335) (Sysix) - 0ec0847 ci: Run napi tests on windows (#14383) (camc314) ### 🐛 Bug Fixes - 11e0440 linter/jsx-handler-name: Improve handler name position in error messages (#14174) (Takuji Shimokawa) - 493082c language_server: Use the first Span of the message as the primary Diagnostic range (#14057) (Sysix) - 88ec1bd linter/plugins: Fix error messages (#14423) (overlookmotel) - 18616c2 oxlint: Ignore fixtures dir for vitest (#14414) (camc314) - ec02fe8 oxlint: Normalize path separators in snapshot tests (#14406) (camc314) - 6e8d2f6 language_server: Ignore JS plugins (#14379) (overlookmotel) - 96663fb linter/plugins: Do not call `before` hook if empty visitor (#14401) (overlookmotel) - 52f04bd linter: Use `pathToFileURL` for importing plugins to ensure correct URL format (#14394) (camc314) - 1ea0d46 oxlint: Resolve tsdown deprecation warning (#14389) (camc314) - bb679b5 linter: Promise/prefer-await-to-then strict option not reading from config (#14382) (camc314) ### 🚜 Refactor - 3374b8e linter/language_server: Move all lsp relevant code to `oxc_language_server` crate (#14430) (Sysix) - d24b74e linter/language_server: `oxc_linter::TsgoLinter::run_source` returns `Message` (#14429) (Sysix) - e5b7fb2 linter/language_server: `oxc_linter::Runtime::run_source` returns `Message` (#14428) (Sysix) - 3b26bf3 linter/plugins: Split adding visit function to compiler visitor into multiple functions (#14433) (overlookmotel) - af3a75e linter/plugins: Track ancestors while walking AST (#14432) (overlookmotel) - f279f0b linter/plugins: Do not lazy-load visitor keys (#14431) (overlookmotel) - 5e99ed3 linter/plugins: Allow nullish values as `message` or `messageId` (#14422) (overlookmotel) - dc30938 linter/plugins: Remove default value from `Context` constructor (#14421) (overlookmotel) - 28cfae0 oxlint: Use `vitest`s built in file snapshot comparison (#14392) (camc314) - 06b0e9f linter/plugins: Convert generated files to TS (#14385) (overlookmotel) - 52f35c6 napi/parser, linter/plugins: Rename `types.js` to `type_ids.js` (#14384) (overlookmotel) ### ⚡ Performance - 26435a1 linter/plugins: Small perf optimizations (#14420) (overlookmotel) - d8a8be1 linter/plugins: Avoid private methods (#14418) (overlookmotel) ### 🧪 Testing - d8da4a4 linter/plugins: Clarify tests for message placeholders (#14417) (overlookmotel) Co-authored-by: camc314 <18101008+camc314@users.noreply.github.com>

Previously we produced an error when JS plugins are defined in config but no
ExternalLinteris present. That's the desired behavior inoxlintCLI, because it happens when user is running Oxlint on an unsupported platform (32-bit, or big-endian).But it's not what we want to happen in language server. Language server does not yet support JS plugins, but user may want to use them in their project (via
oxlintCLI), and language server should not fail with a "JS plugins are not supported" error on any file in such a project.This PR prevents that by adding an
is_enabledfield toExternalPluginStore. By default,is_enabled = true, but language server sets it tofalse. Whenfalse, JS plugins are silently ignored, rather than producing an error.Note: A better solution would be to pass around
Option<ExternalPluginStore>instead ofExternalPluginStorehaving anis_enabledflag. But JS plugin support will be added to the language server pretty soon, at which point we'll need to revert this change.is_enabledflag approach will produce less churn when we do that.