Skip to content

refactor(linter/plugins): add napi function createWorkspace and destroyWorkspace#17809

Merged
graphite-app[bot] merged 1 commit intomainfrom
01-08-chore_oxlint_napi_add_napi_function_createworkspace_and_destroyworkspace_
Jan 30, 2026
Merged

refactor(linter/plugins): add napi function createWorkspace and destroyWorkspace#17809
graphite-app[bot] merged 1 commit intomainfrom
01-08-chore_oxlint_napi_add_napi_function_createworkspace_and_destroyworkspace_

Conversation

@Sysix
Copy link
Member

@Sysix Sysix commented Jan 8, 2026

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:

@github-actions github-actions bot added A-linter Area - Linter A-cli Area - CLI A-linter-plugins Area - Linter JS plugins labels Jan 8, 2026
Copy link
Member Author

Sysix commented Jan 8, 2026


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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.

@github-actions github-actions bot added the C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior label Jan 8, 2026
@codspeed-hq
Copy link

codspeed-hq bot commented Jan 8, 2026

Merging this PR will not alter performance

✅ 46 untouched benchmarks
⏩ 3 skipped benchmarks1


Comparing 01-08-chore_oxlint_napi_add_napi_function_createworkspace_and_destroyworkspace_ (2949b9d) with main (e39a983)2

Open in CodSpeed

Footnotes

  1. 3 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

  2. No successful run was found on main (b34a155) during the generation of this report, so e39a983 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@Sysix Sysix force-pushed the 01-08-refactor_oxlint_lsp_pass_externallinter_-store_to_nested_config_search branch from 1522106 to abc83bc Compare January 8, 2026 21:52
@Sysix Sysix force-pushed the 01-08-chore_oxlint_napi_add_napi_function_createworkspace_and_destroyworkspace_ branch 2 times, most recently from 4624412 to fd1a69a Compare January 8, 2026 22:29
@github-actions github-actions bot added the A-formatter Area - Formatter label Jan 8, 2026
@graphite-app graphite-app bot changed the base branch from 01-08-refactor_oxlint_lsp_pass_externallinter_-store_to_nested_config_search to graphite-base/17809 January 8, 2026 22:30
@graphite-app graphite-app bot force-pushed the 01-08-chore_oxlint_napi_add_napi_function_createworkspace_and_destroyworkspace_ branch from fd1a69a to 77c0e93 Compare January 8, 2026 22:37
@graphite-app graphite-app bot force-pushed the graphite-base/17809 branch from abc83bc to 36b82ec Compare January 8, 2026 22:37
@graphite-app graphite-app bot changed the base branch from graphite-base/17809 to main January 8, 2026 22:37
@graphite-app graphite-app bot force-pushed the 01-08-chore_oxlint_napi_add_napi_function_createworkspace_and_destroyworkspace_ branch from 77c0e93 to d783e7d Compare January 8, 2026 22:38
@Sysix Sysix force-pushed the 01-08-chore_oxlint_napi_add_napi_function_createworkspace_and_destroyworkspace_ branch 2 times, most recently from b52ddea to f0982bf Compare January 8, 2026 22:39
@Sysix Sysix removed the A-formatter Area - Formatter label Jan 8, 2026
@Sysix Sysix force-pushed the 01-08-chore_oxlint_napi_add_napi_function_createworkspace_and_destroyworkspace_ branch 5 times, most recently from a64e84a to a816eec Compare January 9, 2026 21:35
@Sysix Sysix requested a review from Copilot January 10, 2026 00:05
Copy link
Contributor

Copilot AI left a 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 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 cwd access in createOnce hooks, 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.

@Sysix Sysix force-pushed the 01-08-chore_oxlint_napi_add_napi_function_createworkspace_and_destroyworkspace_ branch from a816eec to dd99973 Compare January 10, 2026 12:57
@overlookmotel overlookmotel changed the title chore(oxlint/napi): add napi function createWorkspace and destroyWorkspace refactor(linter/plugins): add napi function createWorkspace and destroyWorkspace Jan 29, 2026
graphite-app bot pushed a commit that referenced this pull request Jan 30, 2026
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`.
@graphite-app graphite-app bot changed the base branch from om/01-29-fix_linter_plugins_ruletester_set_context.filename_to_absolute_path to graphite-base/17809 January 30, 2026 01:14
graphite-app bot pushed a commit that referenced this pull request Jan 30, 2026
… 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.
@graphite-app graphite-app bot force-pushed the graphite-base/17809 branch from b13bd73 to b34a155 Compare January 30, 2026 01:23
@graphite-app graphite-app bot force-pushed the 01-08-chore_oxlint_napi_add_napi_function_createworkspace_and_destroyworkspace_ branch from 2a0a3c7 to dffa2f3 Compare January 30, 2026 01:23
@graphite-app graphite-app bot changed the base branch from graphite-base/17809 to main January 30, 2026 01:23
@graphite-app graphite-app bot force-pushed the 01-08-chore_oxlint_napi_add_napi_function_createworkspace_and_destroyworkspace_ branch from dffa2f3 to 2949b9d Compare January 30, 2026 01:23
@overlookmotel
Copy link
Member

I believe there's a bug here in loadPlugin. It's assuming that the plugin file is within the workspace, but that's not necessarily the case. e.g.:

  • Workspace 1: /path/to/project/packages/foo
  • Workspace 2: /path/to/project/packages/bar
  • JS plugin: /path/to/project/node_modules/eslint-plugin-whatever

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.

@camc314 camc314 added the 0-merge Merge with Graphite Merge Queue label Jan 30, 2026
Copy link
Contributor

camc314 commented Jan 30, 2026

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
@graphite-app graphite-app bot force-pushed the 01-08-chore_oxlint_napi_add_napi_function_createworkspace_and_destroyworkspace_ branch from 2949b9d to 5bd6758 Compare January 30, 2026 13:52
@graphite-app graphite-app bot merged commit 5bd6758 into main Jan 30, 2026
22 checks passed
@graphite-app graphite-app bot deleted the 01-08-chore_oxlint_napi_add_napi_function_createworkspace_and_destroyworkspace_ branch January 30, 2026 13:59
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Jan 30, 2026
graphite-app bot pushed a commit that referenced this pull request Jan 30, 2026
…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.
graphite-app bot pushed a commit that referenced this pull request Jan 31, 2026
Update conformance snapshots. They got out of sync after #17809 (1 broken test, and change to how code is split across files in bundle).
graphite-app bot pushed a commit that referenced this pull request Jan 31, 2026
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.
graphite-app bot pushed a commit that referenced this pull request Jan 31, 2026
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.
graphite-app bot pushed a commit that referenced this pull request Jan 31, 2026
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.
camc314 pushed a commit that referenced this pull request Jan 31, 2026
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-cli Area - CLI A-linter Area - Linter A-linter-plugins Area - Linter JS plugins C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments