-
Notifications
You must be signed in to change notification settings - Fork 0
feat: use free port #14
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
It is mean to be executed manually in dev mode with the browser. Use the env port variable in that case.
src/App.tsx
Outdated
| <div className="App"> | ||
| <h1>Benchttp</h1> | ||
| {isLoading ? <div>Loading</div> : <SimpleStream />} | ||
| {isLoading ? <div>Loading</div> : <SimpleStream port={port} />} |
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.
Not a fan of the extensive drilling for port starting from here: useSpawnEngine ➡️ SimpleStream ➡️ useRunStream ➡️ RunStreamer ➡️ startRunStream ➡️ streamUrl
I think it's a bit much for what appears to be a configuration value, and the signature of these functions should probably not be impacted by the single fact that the port is retrieved at run time.
For these reasons I'd suggest to have an initialization step thay sets the target URL in a globally accessible configuration (e.g. engineConfiguration.baseUrl, or set a new env variable ENGINE_BASE_URL once retrieved, or any better idea) directly accessible from RunStreamer instead.
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.
I thought about it too when making the changes! But I went for the current implem because it was really easy to get it working.
Env variable seems to be a good candidate with low effort required.
Remove the need for useSpawnEngine hook. Remove the ability to get the loading state.
Description
Resolves the related issue by implementing the first solution suggested there. The child process (engine) finds any free port and notifies its parent of the one it found.
This allows for quicker port finding by simply asking the os (see below) instead of trying within a range and locking the port in the parent process until the child is ready to be spawned.
Finding a free port is done by asking the os for port "0", which the os interprets as "any available port".
When running the project in the browser (
npm run web:dev), we cannot rely on dynamic port attribution and the server notifying the frontend client. Instead we use a port defined as an environment variable in.env.developmentand spawn the server with the--any-port=falseflag.Changes
spawnEngineVITE_ENGINE_ADDRESSvariable (prefixed to be exposed in the web view, as per the docs)Linked issues
Resolves #6
Notes