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 VS Code LSP crash #462

Merged
merged 1 commit into from
Aug 19, 2023
Merged

Fix VS Code LSP crash #462

merged 1 commit into from
Aug 19, 2023

Conversation

flying-sheep
Copy link
Contributor

@flying-sheep flying-sheep commented Aug 18, 2023

As mentioned in #434 (comment), getrandom has a workaround built in around the fact that node.js’s ESM story is still not properly solved (rust-random/getrandom#256 (comment))

That workaround exists in getrandom 0.2.8+, but several crates still accepted ahash 0.7.6, which kept the getrandom version at 0.2.7.

This PR bumps the ahash versions we depend on so compatible getrandom versions can be used.

Fixes #434

@flying-sheep
Copy link
Contributor Author

flying-sheep commented Aug 18, 2023

Please feel free to edit this PR until it fullfills all your quality standards. That’s going to be be faster than having me do it, especially when each edit requires you to manually approve workflow runs.

Alternatively, you can change the workflow approval setting from

Fork pull request workflows from outside collaborators

  • Require approval for first-time contributors who are new to GitHub
  • Require approval for first-time contributors

to

Fork pull request workflows from outside collaborators

  • Require approval for first-time contributors who are new to GitHub
  • Require approval for first-time contributors

then I’m happy to work on this until CI passes (unless that would involve releasing a new version of something)

see https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository#controlling-changes-from-forks-to-workflows-in-public-repositories

@tamasfe
Copy link
Owner

tamasfe commented Aug 18, 2023

Thanks! I changed the github settings.

@flying-sheep
Copy link
Contributor Author

flying-sheep commented Aug 18, 2023

Seems like CI passes in any case!

I’m not 100% sure which crates / npm packages need to be compiled and released in which order before the extension can be released with this bugfix, since I used yarn add link:../some/path to use local versions during development. I can only say that before my changes, I could reproduce the issue, and after, I couldn’t anymore. The two necessary changes are

  1. the JS workaround in server.js
  2. Making sure that the WASM embedded in @taplo/lsp contains getrandom 0.2.8 or later

To add tests for this, you’d need to add VS Code extension tests, which I think is out of scope of this PR.

Copy link
Collaborator

@ia0 ia0 left a comment

Choose a reason for hiding this comment

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

Thanks!

@ia0 ia0 requested a review from panekj August 18, 2023 21:52
@panekj panekj merged commit d363946 into tamasfe:master Aug 19, 2023
4 checks passed
@flying-sheep flying-sheep deleted the fix-lsp-crash branch August 19, 2023 20:17
@ia0 ia0 mentioned this pull request Sep 15, 2023
@ia0 ia0 mentioned this pull request Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LSP Server Crashing on VS Code
4 participants