-
-
Notifications
You must be signed in to change notification settings - Fork 712
feat(editor): add oxc.path.node option
#15040
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
base: main
Are you sure you want to change the base?
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. |
|
@EDM115 could you test this option if this works for you with |
|
yes it does :) |
|
once again I know that this is an FNM issue, but with the latest release being nearly a year old I don't expect anything from that side. |
3406ed2 to
06520d7
Compare
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 adds support for configuring a custom Node.js binary path that will be prepended to the language server's PATH environment variable. This allows users to specify a particular Node.js installation to be used by the Oxc language server.
Key changes:
- Added a new
oxc.path.nodeconfiguration option for specifying a Node.js binary path - Implemented the configuration getter/setter in VSCodeConfig
- Modified the server startup logic to prepend the Node.js path to the PATH environment variable
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
editors/vscode/package.json |
Relocated oxc.path.server configuration and added new oxc.path.node configuration property |
editors/vscode/client/VSCodeConfig.ts |
Added nodePath property and related getter/setter methods to VSCodeConfig class and interface |
editors/vscode/client/extension.ts |
Modified server environment setup to include the custom Node.js path in the PATH environment variable |
editors/vscode/tests/VSCodeConfig.spec.ts |
Updated tests to include path.node in test setup/teardown and added assertions for the new property |
editors/vscode/README.md |
Added documentation for the new oxc.path.node configuration option |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
06520d7 to
bea7796
Compare
| RUST_LOG: process.env.RUST_LOG || 'info', | ||
| }; | ||
| if (nodePath) { | ||
| serverEnv.PATH = `${nodePath}${process.platform === 'win32' ? ';' : ':'}${process.env.PATH ?? ''}`; |
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.
The documentation states nodePath is a "Path to a Node.js binary" (e.g., /usr/local/bin/node), but the code adds it directly to PATH without extracting the directory. PATH environment variables should contain directories, not file paths. This will cause the PATH to be malformed if users follow the documentation.
Fix: Extract the directory from the binary path:
if (nodePath) {
const nodeDir = require('path').dirname(nodePath);
serverEnv.PATH = `${nodeDir}${process.platform === 'win32' ? ';' : ':'}${process.env.PATH ?? ''}`;
}Alternatively, update the documentation to clarify that users should provide the directory path, not the binary path.
| serverEnv.PATH = `${nodePath}${process.platform === 'win32' ? ';' : ':'}${process.env.PATH ?? ''}`; | |
| if (nodePath) { | |
| const nodeDir = require('path').dirname(nodePath); | |
| serverEnv.PATH = `${nodeDir}${process.platform === 'win32' ? ';' : ':'}${process.env.PATH ?? ''}`; | |
| } |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.

see #14920 (comment)