Skip to content

refactor(linter): move around some config store logic#10861

Merged
graphite-app[bot] merged 1 commit intomainfrom
c/05-07-refactor_linter_move_around_some_config_store_logic
May 8, 2025
Merged

refactor(linter): move around some config store logic#10861
graphite-app[bot] merged 1 commit intomainfrom
c/05-07-refactor_linter_move_around_some_config_store_logic

Conversation

@camc314
Copy link
Contributor

@camc314 camc314 commented May 7, 2025

this PR moves all of the config resolution for a single file into the ConfigStore structure, rather than half existing in oxc_linter and half in oxlint crates

Copy link
Contributor Author

camc314 commented May 7, 2025


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 A-linter Area - Linter A-cli Area - CLI A-editor Area - Editor and Language Server C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior labels May 7, 2025
@camc314 camc314 changed the base branch from c/05-07-refactor_linter_extract_nested_config_searching_to_a_fn to graphite-base/10861 May 7, 2025 13:11
@codspeed-hq
Copy link

codspeed-hq bot commented May 7, 2025

CodSpeed Instrumentation Performance Report

Merging #10861 will not alter performance

Comparing c/05-07-refactor_linter_move_around_some_config_store_logic (79819cc) with main (ccda8f0)

Summary

✅ 36 untouched benchmarks

@camc314 camc314 changed the base branch from graphite-base/10861 to c/05-07-refactor_linter_extract_nested_config_searching_to_a_fn May 7, 2025 14:22
@camc314 camc314 changed the base branch from c/05-07-refactor_linter_extract_nested_config_searching_to_a_fn to graphite-base/10861 May 7, 2025 14:23
@camc314 camc314 changed the base branch from graphite-base/10861 to c/05-07-refactor_linter_extract_nested_config_searching_to_a_fn May 7, 2025 14:23
@graphite-app graphite-app bot changed the base branch from c/05-07-refactor_linter_extract_nested_config_searching_to_a_fn to graphite-base/10861 May 7, 2025 14:38
@camc314 camc314 force-pushed the c/05-07-refactor_linter_move_around_some_config_store_logic branch from 5ec242f to adb9b1f Compare May 7, 2025 15:20
@camc314 camc314 changed the base branch from graphite-base/10861 to main May 7, 2025 15:20
@camc314 camc314 force-pushed the c/05-07-refactor_linter_move_around_some_config_store_logic branch from e483940 to 7591333 Compare May 7, 2025 15:26
@camc314
Copy link
Contributor Author

camc314 commented May 7, 2025

@Sysix does this approach make sense to you and do you see any concerns around it regarding the language server?

The main point here, is there'x 1x ConfigStore now, and it stores all nested configs, including resolution of getting those configs (applying overrides ect).

Eventually, this will be built via ConfigStoreBuilder - we could return a list of files to tell the language server to watch for changes (thinking config extends ...), at which point, we can just re-build the config store to get the updated configuration

@Sysix
Copy link
Member

Sysix commented May 7, 2025

@camc314 after #10775 only the ServerLinter is responsible for nested configs.
The changes look good for me.

Optimizing the logic for the language server is preferred, but I choose against it for the moment.
The Language Server is working now like this:
Client Request → Language Backend
Language Backend → Searching for the right Worker
Workspace Worker → Requesting / Recreating the Server linter
Server linter → Building nested configs on (re)creation

we could return a list of files to tell the language server to watch for changes (thinking config extends ...), at which point, we can just re-build the config store to get the updated configuration

This is a bigger problem. Because the watchers are currently on the client side. The client should not know the architecture of the linter

@camc314 camc314 force-pushed the c/05-07-refactor_linter_move_around_some_config_store_logic branch from 7591333 to f9c2ed2 Compare May 8, 2025 09:46
@camc314
Copy link
Contributor Author

camc314 commented May 8, 2025

@camc314 after #10775 only the ServerLinter is responsible for nested configs. The changes look good for me.

Optimizing the logic for the language server is preferred, but I choose against it for the moment. The Language Server is working now like this: Client Request → Language Backend Language Backend → Searching for the right Worker Workspace Worker → Requesting / Recreating the Server linter Server linter → Building nested configs on (re)creation

this sounds expensive to me, but I don't fully understand how the language server works.

we could return a list of files to tell the language server to watch for changes (thinking config extends ...), at which point, we can just re-build the config store to get the updated configuration

This is a bigger problem. Because the watchers are currently on the client side. The client should not know the architecture of the linter

Got it. Do you know how other lsps implement this?

@camc314 camc314 marked this pull request as ready for review May 8, 2025 09:49
@camc314 camc314 requested a review from Sysix as a code owner May 8, 2025 09:49
@camc314 camc314 force-pushed the c/05-07-refactor_linter_move_around_some_config_store_logic branch 4 times, most recently from f1e71af to e832938 Compare May 8, 2025 10:57
@Sysix
Copy link
Member

Sysix commented May 8, 2025

this sounds expensive to me, but I don't fully understand how the language server works.

It is, but this should only be noticeable when changing configuration / oxlintrc.json.
We can improve it in the future :)

Do you know how other lsps implement this?

I know the most LSP will create Server File Watchers. Maybe some can tell the client to watch for files with registerCapabability?.
Creating File Watchers on the server side is not recommended:

Servers are allowed to run their own file system watching mechanism and not rely on clients to provide file system events. However this is not recommended due to the following reasons:

  • to our experience getting file system watching on disk right is challenging, especially if it needs to be supported across multiple OSes.
  • file system watching is not for free especially if the implementation uses some sort of polling and keeps a file system tree in memory to compare time stamps (as for example some node modules do)
  • a client usually starts more than one server. If every server runs its own file system watching it can become a CPU or memory problem.
  • in general there are more server than client implementations. So this problem is better solved on the client side.

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_didChangeWatchedFiles

@camc314 camc314 force-pushed the c/05-07-refactor_linter_move_around_some_config_store_logic branch from e832938 to e52b015 Compare May 8, 2025 11:14
@camc314 camc314 added the 0-merge Merge with Graphite Merge Queue label May 8, 2025
Copy link
Contributor Author

camc314 commented May 8, 2025

Merge activity

this PR moves all of the config resolution for a single file into the `ConfigStore` structure, rather than half existing in `oxc_linter` and half in `oxlint` crates
@graphite-app graphite-app bot force-pushed the c/05-07-refactor_linter_move_around_some_config_store_logic branch from e52b015 to 79819cc Compare May 8, 2025 11:21
@graphite-app graphite-app bot merged commit 79819cc into main May 8, 2025
28 checks passed
@graphite-app graphite-app bot deleted the c/05-07-refactor_linter_move_around_some_config_store_logic branch May 8, 2025 11:27
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label May 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-cli Area - CLI A-editor Area - Editor and Language Server A-linter Area - Linter 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.

2 participants