Skip to content

feat(editor): add custom node path settings #11018

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.

Sign up for GitHub

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

Jump to bottom
Closed
Sysix wants to merge 1 commit into from
Closed

feat(editor): add custom node path settings #11018

Sysix wants to merge 1 commit into from

Conversation

@Sysix
Copy link
Member

@Sysix Sysix commented May 13, 2025 •
edited
Loading

closes #9925

@Sysix Sysix mentioned this pull request May 13, 2025
Merged
@Sysix Graphite App
Copy link
Member Author

Sysix commented May 13, 2025 •
edited
Loading

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 A-editor Area - Editor and Language Server label May 13, 2025
@Sysix Sysix mentioned this pull request May 13, 2025
Merged
@github-actions github-actions bot added the C-enhancement Category - New feature or request label May 13, 2025
@Sysix Sysix force-pushed the 05-13-feat_editor_add_custom_node_path_settings branch from 0106010 to c1e2e63 Compare May 13, 2025 16:52
camc314
camc314 approved these changes May 15, 2025
View reviewed changes
Copy link
Contributor

@camc314 camc314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks great

@graphite-app graphite-app bot changed the base branch from 05-13-fix_editor_send_only_workspace_didchangeconfiguration_when_some_workspace_configuration_is_effected to graphite-base/11018 May 15, 2025 08:40
@graphite-app graphite-app bot force-pushed the 05-13-feat_editor_add_custom_node_path_settings branch from c1e2e63 to 549e417 Compare May 15, 2025 08:44
@graphite-app graphite-app bot force-pushed the graphite-base/11018 branch from 56e36fc to 87bf2a8 Compare May 15, 2025 08:44
@graphite-app graphite-app bot changed the base branch from graphite-base/11018 to main May 15, 2025 08:45
@graphite-app graphite-app bot force-pushed the 05-13-feat_editor_add_custom_node_path_settings branch from 549e417 to 2dad462 Compare May 15, 2025 08:45
@Sysix Sysix force-pushed the 05-13-feat_editor_add_custom_node_path_settings branch 2 times, most recently from 8efbdef to b93e4fd Compare May 16, 2025 09:25
@Sysix
Copy link
Member Author

Sysix commented May 16, 2025

@camc314 I could not get a crash with the following configuration:

{
  "oxc.path.server": "/home/sysix/dev/oxc/editors/vscode/node_modules/.bin/oxc_language_server",
  "oxc.path.node": "/no-valid-path"
}

I looked into the ESLint VS Code plugin and the eslint.nodePath is passed to the server and the server is doing some stuff.
We are just using this for the JS environment detection before starting the server.

Not really sure if this will resolve the issue :/

@Sysix Sysix marked this pull request as ready for review May 16, 2025 09:32
@Sysix Sysix requested a review from camc314 May 16, 2025 09:32
graphite-app[bot]
graphite-app bot reviewed May 16, 2025
View reviewed changes
@Sysix Sysix force-pushed the 05-13-feat_editor_add_custom_node_path_settings branch from b93e4fd to dd7b9b6 Compare May 16, 2025 09:37
@camc314 camc314 self-assigned this May 16, 2025
camc314
camc314 reviewed May 16, 2025
View reviewed changes
editors/vscode/client/extension.ts Outdated
if (configService.vsCodeConfig.nodePath) {
outputChannel.info(`adding path to NODE_PATH: ${configService.vsCodeConfig.nodePath}`);
if (serverEnv.NODE_PATH) {
serverEnv.NODE_PATH = serverEnv.NODE_PATH.concat(path.delimiter, configService.vsCodeConfig.nodePath);
Copy link
Contributor

@camc314 camc314 May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we not be appending this to PATH ?

Copy link
Contributor

@camc314 camc314 May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for reference, TS extension calls child_process.spawn if a custom node path is provided

https://github.com/microsoft/vscode/blob/b1ecfe96b5c861255ad5beb34cf48465ef43f9ef/extensions/typescript-language-features/src/tsServer/serverProcess.electron.ts#L280-L292

Copy link
Member Author

@Sysix Sysix May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it to PATH.
I do not understand that some of VSCode users are using the node_modules language_server, which has the JS wrapper.
I am always using the direct binary with custom oxc.path.server or from the VSCode extension.

@Sysix Sysix force-pushed the 05-13-feat_editor_add_custom_node_path_settings branch from dd7b9b6 to 7d23fcb Compare May 16, 2025 12:00
graphite-app[bot]
graphite-app bot reviewed May 16, 2025
View reviewed changes
@Sysix Sysix force-pushed the 05-13-feat_editor_add_custom_node_path_settings branch from 7d23fcb to 8e576a0 Compare May 16, 2025 12:03
@Sysix Sysix marked this pull request as draft May 18, 2025 12:18
@Sysix Sysix mentioned this pull request May 18, 2025
Merged
graphite-app bot pushed a commit that referenced this pull request May 18, 2025
@Sysix
refactor(editor): use always the provided language server (#11115)
a28fe1e 
Now, the editor will not look into `node_modules` folder to use a custom `oxc_language_server`.
The editor will always use the provided language server.

## Reasons

Because the language server in `node_modules` could be a different version than the extension and changes like #10890 can break the communication between them.

Avoiding the JS Wrapper and resolving #9925 and avoiding a new setting #11018

closes #9925
closes #11018
@Sysix Sysix closed this May 18, 2025
@Sysix Sysix deleted the 05-13-feat_editor_add_custom_node_path_settings branch July 11, 2025 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@camc314 camc314 camc314 approved these changes

@graphite-app graphite-app[bot] graphite-app[bot] left review comments

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

@camc314 camc314

Labels

A-editor Area - Editor and Language Server C-enhancement Category - New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

linter: oxc_language_server can't find node runtime (VSCode extension)

3 participants

@Sysix @camc314