refactor(linter/plugins): add napi function createWorkspace and destroyWorkspace#17809
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. |
Merging this PR will not alter performance
Comparing Footnotes
|
1522106 to
abc83bc
Compare
4624412 to
fd1a69a
Compare
fd1a69a to
77c0e93
Compare
abc83bc to
36b82ec
Compare
77c0e93 to
d783e7d
Compare
b52ddea to
f0982bf
Compare
a64e84a to
a816eec
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds workspace management capabilities to the oxlint NAPI bindings, introducing createWorkspace and destroyWorkspace functions. The changes enable proper isolation of plugin dependencies across different workspace directories, and update the context API to pass the current working directory (cwd) through the configuration setup chain.
Key changes:
- Added NAPI callbacks for workspace creation and destruction with corresponding Rust and TypeScript implementations
- Modified the context API to reject
cwdaccess increateOncehooks, requiring explicit workspace setup - Updated plugin loading to be workspace-aware, storing rules per workspace instead of globally
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/oxc_linter/src/lib.rs | Exports new workspace callback types |
| crates/oxc_linter/src/external_linter.rs | Adds workspace creation/destruction callbacks to ExternalLinter struct |
| crates/oxc_linter/src/external_plugin_store.rs | Adds cwd parameter to setup_configs method |
| apps/oxlint/src/run.rs | Defines NAPI callback types for workspace management |
| apps/oxlint/src/result.rs | Adds JsPluginWorkspaceSetupFailed result variant |
| apps/oxlint/src/lsp/server_linter.rs | Integrates workspace lifecycle into LSP server |
| apps/oxlint/src/lint.rs | Calls workspace setup during CLI initialization |
| apps/oxlint/src/js_plugins/external_linter.rs | Implements Rust wrappers for workspace callbacks |
| apps/oxlint/src-js/workspace/index.ts | New workspace management module |
| apps/oxlint/src-js/plugins/context.ts | Updates context to enforce workspace requirements for cwd access |
| apps/oxlint/src-js/plugins/load.ts | Makes plugin loading workspace-aware |
| apps/oxlint/src-js/plugins/lint.ts | Updates linting to be workspace-aware |
| apps/oxlint/src-js/plugins/options.ts | Adds workspace validation to options setup |
| apps/oxlint/src-js/cli.ts | Integrates workspace callbacks into CLI entry point |
| apps/oxlint/src-js/bindings.d.ts | Adds TypeScript definitions for workspace callbacks |
| apps/oxlint/src-js/package/rule_tester.ts | Updates rule tester to use workspace APIs |
| apps/oxlint/test/tokens.test.ts | Updates test to pass cwd to setupFileContext |
| apps/oxlint/test/isSpaceBetween.test.ts | Updates test to pass cwd to setupFileContext |
| apps/oxlint/test/fixtures/createOnce/plugin.ts | Updates test to verify cwd access restrictions |
| apps/oxlint/test/fixtures/createOnce/output.snap.md | Updates expected output for cwd error messages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a816eec to
dd99973
Compare
createWorkspace and destroyWorkspacecreateWorkspace and destroyWorkspace
Pass `cwd` into `setupFileContext`, so it can be set per file, rather than being a global setting. This is preamble to #17809. This PR is some of the changes from that one split off, so the diff on #17809 is focused just on that one change. This PR does nothing in itself, as currently `process.cwd()` is always passed to `setupFileContext`.
… path (#18702) `RuleTester` accepts `filename` as a property of test cases. Previously it passed `filename` directly through to the linter, but that wasn't right - the linter expects it to be passed an absolute path. Instead, convert `filename` to an absolute path, if it isn't one already. Not using `process.cwd()` as the base of the path, as `RuleTester` is a testing utility, and the results should be deterministic, unaffected by external factors like CWD. Use the root of `oxlint` package as the base instead (a bit arbitrary, but what would be better?). This is preamble to #17809. This PR is some of the changes from that one split off, so the diff on #17809 is focused just on that one change.
b13bd73 to
b34a155
Compare
2a0a3c7 to
dffa2f3
Compare
dffa2f3 to
2949b9d
Compare
|
I believe there's a bug here in
I don't propose that we try to fix this in this PR - I think better to merge this stack and then iterate on it, but just making this comment here to describe the issue. |
Merge activity
|
…stroyWorkspace` (#17809) This PR introduces workspace isolation for the oxlint NAPI plugin system, adding `createWorkspace` and `destroyWorkspace` functions to manage multi workspace support for the language server. This does not fully solve the integration, but should be a good start for all future work. One example is the buffer for multiple calls at the same time (?). Added a new workspace isolation layer that uses the current working directory (cwd) as the workspace identifier: - `createWorkspace(cwd: string)`: Initializes a new isolated workspace for plugins and configurations - `destroyWorkspace(cwd: string)`: Cleans up workspace resources, unloading all associated plugins and options - `getResponsibleWorkspace(filePath: string)`: Determines which workspace is responsible for a given file path, reflecting the changes in #17853
2949b9d to
5bd6758
Compare
…18727) #17809 and #17840 add support for JS plugins to LSP. However: * We don't have much test coverage yet. * The support for workspaces imposes a perf cost on Oxlint CLI (frequent hash map lookups on JS side). * I believe there's a bug in JS-side code - #17809 (comment). I think it'd be easier to merge this stack and then iterate on it in further PRs to fix the above. To prevent any breakage for users in the meantime, this PR disables JS plugin support in LSP unless the `testing` Cargo feature is enabled - i.e. it's only enabled in tests for now.
Update conformance snapshots. They got out of sync after #17809 (1 broken test, and change to how code is split across files in bundle).
Add a `cwd` option to `RuleTester`. Use this option in conformance tests to set `cwd` to the test files directory. This fixes the 1 test which #17809 broke.
Workspaces support (#17809) added overhead on JS side of hashmap lookups. Every call to `lintFile` has to look up details (e.g. registered rules, rule options) from hashmaps keyed by workspace URI. ### Perf Take a different approach which is optimized for common case of there being only 1 workspace: * At any time there's 1 "current workspace". * Store rules, options, and CWD in top-level vars (back to how it was before #17809). * Switch workspaces only when required. Switching reassigns the values in these top level vars. Before: * Get rules, options, and CWD from hashmaps keyed by workspace URI. After: * Quick check "is the current workspace the one we need?". * If so, do nothing - the top-level vars `registeredRules`, `allOptions`, and `cwd` elready contain the right settings for the desired workspace. * If current workspace is not the right one, look up the details for desired workspace in `workspaces` hash map and write them into the top-level vars. In CLI, `workspaceUri` passed to `lintFile` etc is `null`, so the quick "in the right workspace?" check is particularly cheap - just `null === null`. So this reduces the overhead that workspace support adds to CLI to just that 1 instruction. Add of these functions are entirely synchronous, so there's no possibility of a race where 1 call switches out the values in the top-level vars, while another call is busy using them. ### CLI: No call to `createWorkspace` CLI now doesn't need to call `createWorkspace` at all. The "are we in right workspace?" check always succeeds, so it's irrelevant whether a workspace has been created or not - `workspaces` hashmap is never accessed. So workspaces is now an LSP-only thing. ### Bundle This PR also removes all imports from other files from `src-js/workspace/index.ts`. It now only imports `debugAssert`, which is optimized out in release build., leaving no imports at all. Previously, the imports from other files were causing most of the JS plugins code to get loaded on first call to `createWorkspace`, which broke the lazy loading optimization - Oxlint CLI and LSP were both loading all the JS plugins code eagerly even if the user doesn't use JS plugins. With this change, the lazy loading optimization is restored. This is visible in the conformance snapshot change in this PR.
Workspaces support (#17809) added overhead on JS side of hashmap lookups. Every call to `lintFile` has to look up details (e.g. registered rules, rule options) from hashmaps keyed by workspace URI. ### Perf Take a different approach which is optimized for common case of there being only 1 workspace: * At any time there's 1 "current workspace". * Store rules, options, and CWD in top-level vars (back to how it was before #17809). * Switch workspaces only when required. Switching reassigns the values in these top level vars. Before: * Get rules, options, and CWD from hashmaps keyed by workspace URI. After: * Quick check "is the current workspace the one we need?". * If so, do nothing - the top-level vars `registeredRules`, `allOptions`, and `cwd` elready contain the right settings for the desired workspace. * If current workspace is not the right one, look up the details for desired workspace in `workspaces` hash map and write them into the top-level vars. In CLI, `workspaceUri` passed to `lintFile` etc is `null`, so the quick "in the right workspace?" check is particularly cheap - just `null === null`. So this reduces the overhead that workspace support adds to CLI to just that 1 instruction. Add of these functions are entirely synchronous, so there's no possibility of a race where 1 call switches out the values in the top-level vars, while another call is busy using them. ### CLI: No call to `createWorkspace` CLI now doesn't need to call `createWorkspace` at all. The "are we in right workspace?" check always succeeds, so it's irrelevant whether a workspace has been created or not - `workspaces` hashmap is never accessed. So workspaces is now an LSP-only thing. ### Bundle This PR also removes all imports from other files from `src-js/workspace/index.ts`. It now only imports `debugAssert`, which is optimized out in release build., leaving no imports at all. Previously, the imports from other files were causing most of the JS plugins code to get loaded on first call to `createWorkspace`, which broke the lazy loading optimization - Oxlint CLI and LSP were both loading all the JS plugins code eagerly even if the user doesn't use JS plugins. With this change, the lazy loading optimization is restored. This is visible in the conformance snapshot change in this PR.
Workspaces support (#17809) added overhead on JS side of hashmap lookups. Every call to `lintFile` has to look up details (e.g. registered rules, rule options) from hashmaps keyed by workspace URI. ### Perf Take a different approach which is optimized for common case of there being only 1 workspace: * At any time there's 1 "current workspace". * Store rules, options, and CWD in top-level vars (back to how it was before #17809). * Switch workspaces only when required. Switching reassigns the values in these top level vars. Before: * Get rules, options, and CWD from hashmaps keyed by workspace URI. After: * Quick check "is the current workspace the one we need?". * If so, do nothing - the top-level vars `registeredRules`, `allOptions`, and `cwd` elready contain the right settings for the desired workspace. * If current workspace is not the right one, look up the details for desired workspace in `workspaces` hash map and write them into the top-level vars. In CLI, `workspaceUri` passed to `lintFile` etc is `null`, so the quick "in the right workspace?" check is particularly cheap - just `null === null`. So this reduces the overhead that workspace support adds to CLI to just that 1 instruction. Add of these functions are entirely synchronous, so there's no possibility of a race where 1 call switches out the values in the top-level vars, while another call is busy using them. ### CLI: No call to `createWorkspace` CLI now doesn't need to call `createWorkspace` at all. The "are we in right workspace?" check always succeeds, so it's irrelevant whether a workspace has been created or not - `workspaces` hashmap is never accessed. So workspaces is now an LSP-only thing. ### Bundle This PR also removes all imports from other files from `src-js/workspace/index.ts`. It now only imports `debugAssert`, which is optimized out in release build., leaving no imports at all. Previously, the imports from other files were causing most of the JS plugins code to get loaded on first call to `createWorkspace`, which broke the lazy loading optimization - Oxlint CLI and LSP were both loading all the JS plugins code eagerly even if the user doesn't use JS plugins. With this change, the lazy loading optimization is restored. This is visible in the conformance snapshot change in this PR.

This PR introduces workspace isolation for the oxlint NAPI plugin system, adding
createWorkspaceanddestroyWorkspacefunctions to manage multi workspace support for the language server.This does not fully solve the integration, but should be a good start for all future work. One example is the buffer for multiple calls at the same time (?).
Added a new workspace isolation layer that uses the current working directory (cwd) as the workspace identifier:
createWorkspace(cwd: string): Initializes a new isolated workspace for plugins and configurationsdestroyWorkspace(cwd: string): Cleans up workspace resources, unloading all associated plugins and optionsgetResponsibleWorkspace(filePath: string): Determines which workspace is responsible for a given file path, reflecting the changes in fix(lsp): fix workspace worker selection for nested and similar-named workspaces #17853