Skip to content
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

fix(vscode): server bin path resolution #227

Merged
merged 6 commits into from
Sep 14, 2023
Merged

fix(vscode): server bin path resolution #227

merged 6 commits into from
Sep 14, 2023

Conversation

nhedger
Copy link
Member

@nhedger nhedger commented Sep 10, 2023

Summary

This PR fixes an issue where the VS Code extension cannot resolve the path to the biome CLI when using certain package managers, notably pnpm.

The new strategy is to point to the node_modules/.bin/biome path, which is consistent for all package managers.

Test Plan

  • Tested working with npm on:
    • macOS
    • Windows
    • Linux
  • Tested working with pnpm on:
    • macOS
    • Windows
    • Linux
  • Tested working with yarn on:
    • macOS
    • Windows
    • Linux
  • Tested working with bun on:
    • macOS
    • Windows (not applicable at the moment)
    • Linux

I'd like some help to test under Windows, I don't have a machine handy.

@nhedger nhedger changed the title fix: server bin path resolution fix(vscode): server bin path resolution Sep 10, 2023
@fox1t
Copy link

fox1t commented Sep 12, 2023

Could you add an automation test on the CI to test this code properly?

@nhedger
Copy link
Member Author

nhedger commented Sep 12, 2023

Could you add an automation test on the CI to test this code properly?

It is technically possible, and while I'm usually all for it, this would require a non-trivial amount of additional work, for which I'm not willing to invest time at the moment.

Considering that the VS Code extension rarely changes, I think that doing a manual test will suffice for now. Feel free to PR something if/when this is merged.

@SuperchupuDev
Copy link
Member

what's left for this pull request to be considered ready for review? i'd love it if it managed to get included in the next biome release

@nhedger
Copy link
Member Author

nhedger commented Sep 13, 2023

It needs to be tested under Windows and Linux.

I'll come up with a detailed test plan tomorrow.

@nhedger
Copy link
Member Author

nhedger commented Sep 14, 2023

Alright, it works as expected on Linux. I'll be testing on a Windows VM today and then we should be good to go.

@@ -363,6 +363,7 @@ async function getSocket(
): Promise<string> {
const process = spawn(command, ["__print_socket"], {
stdio: [null, "pipe", "pipe"],
shell: true,
Copy link
Member Author

Choose a reason for hiding this comment

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

This is required to spawn the biome binary in node_modules/.bin under windows.

@nhedger
Copy link
Member Author

nhedger commented Sep 14, 2023

A little adjustment was needed to make it work with Windows, but it should be all good now. I'll mark the PR as ready and will let you review the code.

@nhedger nhedger marked this pull request as ready for review September 14, 2023 13:54
"watch": "pnpm run compile -- --sourcemap --watch",
"watch": "pnpm run compile --sourcemap --watch",
Copy link
Member Author

Choose a reason for hiding this comment

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

Missed this one in #219.

Let me know if you'd rather I submit this in a separate PR.

@NumberOneBot
Copy link
Contributor

Guys, when do you plan to add this into release? I'm trying to incorporate biome in my company's processes with several Windows machines.

(Found this thread, trying to make exactly the same pull request.)

@Conaclos Conaclos added this to the Biome 1.2 milestone Sep 14, 2023
@ematipico
Copy link
Member

ematipico commented Sep 14, 2023

Guys, when do you plan to add this into release? I'm trying to incorporate biome in my company's processes with several Windows machines.

(Found this thread, trying to make exactly the same pull request.)

We don't have dates for you. v1.2 will be out soon. We can release a nightly tomorrow, so you can test it and see if that works for you.

@ematipico ematipico merged commit 84770ca into biomejs:main Sep 14, 2023
@nhedger nhedger deleted the fix/server-bin-path-resolution branch September 14, 2023 16:12
@Conaclos Conaclos added the A-Changelog Area: changelog label Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants