-
Notifications
You must be signed in to change notification settings - Fork 14
Introduce runtime abstraction for faster initialization #49
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
Conversation
This commit introduces a new `Runtime` abstraction to manage execution environments for the language server and package installation. The `Runtime` enum currently supports `Bun` (if found on the system path) and falls back to Zed's built-in `Node.js` runtime. This change allows the extension to leverage Bun for potentially faster package management and server execution when available, while maintaining compatibility with the standard Node.js environment. The following changes were made: - Add `src/runtime.rs` defining the `Runtime` enum and its methods (`new`, `server_command`, `install_package`, `latest_package_version`, `installed_package_version`). - Update `Cargo.toml` to include `serde` and `which` dependencies. - Refactor `src/svelte.rs` to use the new `Runtime` for package installation checks, version lookups, and starting the language server process.
|
We require contributors to sign our Contributor License Agreement, and we don't have @realfakenerd on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
1 similar comment
|
We require contributors to sign our Contributor License Agreement, and we don't have @realfakenerd on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
@cla-bot check |
|
We require contributors to sign our Contributor License Agreement, and we don't have @realfakenerd on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
MrSubidubi
left a comment
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.
Thanks for this! I am tempted to hold back on Bun support before we have better support for different runtimes in core (which in turn we could then properly expose to extensions). Curious about your opinion on this, but this feels more like a hack than a proper solution (although no support is obviously also not optimal).
|
I thought about bun because of the obvious improvement in speed, and yes i also think is a hack, but wasn't #23 a hack too, i made the But aside that, the code itself, is looking ... right? :) |
|
It very much was, but we raised an upstream issue about it and proceeded to fix it in core - at the time of that coming up, we did not have any resources for Windows yet, so we allowed that for the time being. As for this, I think this should come down to toolchain selection, which we introduced for Python and should expose/roll out to extensions at some point. So, yeah, you are right to some point, I'd very much prefer for this to be exposed but that might still take some time. Code-wise, I'll leave some comments. |
src/runtime.rs
Outdated
|
|
||
| pub fn latest_package_version(&self, package_name: &str) -> Result<String>{ | ||
| match self { | ||
| Runtime::Bun(_) | Runtime::Node => zed::npm_package_latest_version(package_name) |
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.
This will be problematic - we will check for the installed version in the Node environment, but then proceed to only install it in the Bun environment (which is obviously right). That means that we will install it in Bun always, even if it is installed.
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.
Hm... i don't think it will be problematic, since both operate on the same place, i think it is like i being node and you being bun, we are searching for a box inside the same room.
|
Also, please do not get me wrong with all this, very grateful for your contributions here! So thanks for taking the time to come up with these! |
Exposed? What do you mean by this? |
Thank you to for the comments, i have some ideas to make it looks more like the VSCode extension, i have made an issue (#51) about slash commands, take a look at it when you have time :) |
The custom `Runtime` logic, which attempted to detect and use Bun, is removed.
|
Closing, i think that for organization purposes it will be better to open a new PR for this in a new branch. |
Added
Runtimeto chose between Node.js and Bun, if bun is installed it uses bun runtime or else uses nodejs.