-
Notifications
You must be signed in to change notification settings - Fork 335
fix(cli): better runtime handling for ui:start #3340
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
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 improves runtime handling for the ui:start
command by implementing better detection and configuration for different JavaScript runtimes (Node.js, Bun, and compiled binaries). The goal is to resolve compatibility issues when spawning the UI server process across different execution environments.
- Adds comprehensive runtime detection for Node.js, Bun, and compiled binaries
- Implements proper spawn configuration with platform-specific handling
- Improves error handling and validation for runtime execution paths
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
runtime-detector.ts |
Implements runtime detection logic with support for Node.js, Bun, and compiled binary detection |
spawn-config.ts |
Provides spawn configuration building and executable path validation |
ui-start.ts |
Updates the UI start command to use the new runtime detection and spawn configuration |
runtime-detector_test.ts |
Comprehensive test coverage for runtime detection functionality |
spawn-config_test.ts |
Test coverage for spawn configuration building and validation |
Seems like it doesn't work right now on Windows at least, investigating further. |
@pavelgj is OOO right now, so I am going to tap in @apascal07 to help with the review if possible. One thing that jumps out to me, without going too deep, we use the term "runtime" in two different contexts now. The "Genkit" runtime and the "Javascript" runtime. So we should at least make sure the error messages refer to the Javascript runtime so that its clear to us/to the user what this is. Potentially we could consider naming the utility classes/methods accordingly as well. |
Will review once it's all there, thanks! |
The windows issues are fixed, I'll rename the classes and logs etc, should be ready for review end of day tomorrow |
4e8fdc5
to
953a322
Compare
This should resolve #3338
Checklist (if applicable):