feat(vscode): fallback to globally installed oxlint/oxfmt packages #18007
Conversation
github-actions
bot
added
A-editor
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
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. |
Sysix
force-pushed
the
01-14-feat_vscode_fallback_to_globally_installed_oxlint_oxfmt_packages
branch
from
cb0b104 to
f9d4c26
Compare
Sysix
force-pushed
the
01-14-feat_vscode_fallback_to_globally_installed_oxlint_oxfmt_packages
branch
from
f9d4c26 to
1f56b0c
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds fallback functionality to search for oxlint/oxfmt binaries in global node_modules when they are not found in project node_modules. The implementation is extracted from ConfigService into a dedicated findBinary module.
Changes:
- Created new
findBinary.tsmodule with functions to search for binaries in project node_modules, global node_modules, and user settings - Refactored ConfigService to use the new findBinary functions with global fallback support
- Added unit tests for the new findBinary functions
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| editors/vscode/client/findBinary.ts | New module containing binary search logic with global node_modules fallback, extracted and enhanced from ConfigService |
| editors/vscode/client/ConfigService.ts | Refactored to use new findBinary module functions, simplified searchBinaryPath method |
| editors/vscode/tests/unit/findBinary.spec.ts | New unit tests for the findBinary module functions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!path.isAbsolute(settingsBinary)) { | ||
| const cwd = workspace.workspaceFolders?.[0]?.uri.fsPath; |
There was a problem hiding this comment.
The refactored code uses workspace.workspaceFolders?.[0]?.uri.fsPath whereas the original ConfigService used this.workspaceConfigs.keys().next().value. This changes the behavior - the original used the first workspace config key (uri.path), while the new code uses the first workspace folder's fsPath. This could lead to different behavior on Windows where path vs fsPath differ in format (forward slashes vs backslashes, drive letter format).
| if (!path.isAbsolute(settingsBinary)) { | ||
| const cwd = workspace.workspaceFolders?.[0]?.uri.fsPath; | ||
| if (!cwd) { | ||
| return undefined; | ||
| } | ||
| // if the path is not absolute, resolve it to the first workspace folder | ||
| settingsBinary = path.normalize(path.join(cwd, settingsBinary)); |
There was a problem hiding this comment.
The original ConfigService had a removeWindowsLeadingSlash call that has been removed in the refactoring. This was used after path.normalize(path.join(...)) to strip leading backslashes on Windows. Without this, the behavior on Windows may differ from the original implementation.
| await workspace.fs.stat(Uri.file(settingsBinary)); | ||
| return settingsBinary; | ||
| } catch {} | ||
|
|
There was a problem hiding this comment.
The original ConfigService code had logic to remove .exe extension on non-Windows platforms before the stat check. The refactored code removes this check, which could affect cross-platform compatibility if settings contain .exe extensions on non-Windows platforms.
| // On non-Windows platforms, if the configured path ends with `.exe`, | |
| // also try the same path without the extension for cross-platform settings. | |
| if (process.platform !== "win32" && settingsBinary.endsWith(".exe")) { | |
| const settingsBinaryWithoutExe = settingsBinary.slice(0, -4); | |
| try { | |
| await workspace.fs.stat(Uri.file(settingsBinaryWithoutExe)); | |
| return settingsBinaryWithoutExe; | |
| } catch {} | |
| } |
| // this depends on the binary being installed in the oxc project's node_modules | ||
| test("should replace dist/index.js with bin/<binary-name> in resolved path", async () => { |
There was a problem hiding this comment.
This test depends on external state (oxlint being installed). The test should either use a mock/fixture binary or be marked as an integration test rather than a unit test, since it requires the actual binary to be present in node_modules.
| }); | ||
|
|
||
| // Skipping this test as it may depend on the actual global installation of the binary | ||
| test.skip("should replace dist/index.js with bin/<binary-name> in resolved path", async () => { |
There was a problem hiding this comment.
The global node modules test is skipped. Consider adding a test that mocks the global resolution behavior to ensure the logic works correctly without depending on actual global installations.
|
|
||
| try { | ||
| const result = spawnSync(command, args, { | ||
| shell: true, |
There was a problem hiding this comment.
Using shell: true can introduce security risks if not carefully managed. While the comment on line 103-104 acknowledges this and states the function is only for internal code, consider whether shell execution is necessary here. The commands 'npm' and 'pnpm' with static args '["root", "-g"]' could likely be executed without shell mode for better security.
| shell: true, |
closes #18005