fix(native): stop implicit dev server startup in setupEmulator#205
fix(native): stop implicit dev server startup in setupEmulator#205AshrefRaj wants to merge 6 commits into
Conversation
This fixes issue tata1mg#204 where setupEmulator commands would leave a background dev server running silently. Added an optional timed prompt (10s) for IP updates instead.
Refactored setupEmulator to include optional interactive prompts for IP configuration and dev server startup. This addresses issue tata1mg#204 by removing implicit background processes while maintaining a low-friction setup flow.
|
Claude review
|
mayankmahavar1mg
left a comment
There was a problem hiding this comment.
We should use exisiting prompt user inplace of timeout.
few functions are used in buildApp script as well, where they take care of specific environment, same utility function will not work there, so either we add as todo comment or handle them as well
Reading and writing file takes time , try minimise this.
| progress.complete("platform") | ||
|
|
||
| progress.start("config") | ||
| const { WEBVIEW_CONFIG } = await initializeConfig() |
There was a problem hiding this comment.
Why removed WEBVIEW_CONFIG ?
we are passing it further in different function.
| progress.start("setupServer") | ||
| await setupServer(configPath) | ||
| progress.start("setupServer") | ||
| await handlePostSetupSteps("ios", configPath, progress, { |
There was a problem hiding this comment.
is this also going to save updated config?
we just save config above at line 54
we should have updated config and save them all in one go
move this section before save config and then call save config with updated config
| }) | ||
| } | ||
|
|
||
| async function promptUserWithTimeout(question, timeoutMs, defaultValue) { |
There was a problem hiding this comment.
can we use existing prompt function
| progress.log(`Dev server already running on port ${port}`, "info") | ||
| } else { | ||
| progress.pause() | ||
| const startServer = await promptUserWithTimeout( |
There was a problem hiding this comment.
prompt should be asked upfront, then only we should read config and all
|
|
||
| // Step 2: Optional Server Start | ||
| progress.start("startServer") | ||
| const configFile = fs.readFileSync(configPath, "utf8") |
There was a problem hiding this comment.
we are reading file again, we should pass related config
Mrsandeep27
left a comment
There was a problem hiding this comment.
Nice factoring — pulling initializeConfig/saveConfig/handlePostSetupSteps into utils.js removes duplication, and moving dev-server start behind an opt-in prompt addresses #204.
A few observations worth considering before merge:
1. handlePostSetupSteps reads + parses config.json twice (once in saveConfig, once here for port). If the file was just written by saveConfig, you already have the updated object in memory — passing it through would avoid another round-trip and a narrow window where a concurrent edit could desync the two reads.
2. iOS and Android save flow are now out of sync. In androidSetup.js the flow is saveConfig(...) → handlePostSetupSteps(...), but in setupEmulatorIos.js it's progress.start("saveConfig") → validateAndCompleteConfig → saveConfig(...) → complete. Worth confirming that the iOS path actually persists WEBVIEW_CONFIG before handlePostSetupSteps reads from disk for port lookup.
3. Default timeout of 10s for promptUserWithTimeout is short. On a first-time setup a user is unlikely to read the prompt, decide, and answer in 10 seconds. Silently defaulting to "n" is safe for #204 but surprising UX — consider 30s, or no timeout when stdin is a TTY.
4. startServerBackground() with the current readline still open. rl is created at module load in utils.js. After the last promptUserWithTimeout, rl is never closed, so the process will hang on exit even when the user selects "n" (skip server). Calling rl.close() at the end of handlePostSetupSteps (or in a finally in each setup entry point) would let the command exit cleanly, which is exactly what #204 is asking for.
5. Minor: const { getLocalIPAddress, ... } = require("./setupServer.js") in setupEmulatorIos.js mixes require with ESM import at the top of the file. The Android file uses an import for setupServer.js helpers — worth making them consistent.
Overall direction looks right. Fixing #4 is probably the most important bit — otherwise the server setup skips launching dev server but the command still doesn't exit.
This fixes issue #204 where setupEmulator commands would leave a background dev server running silently. Added an optional timed prompt (10s) for IP updates instead.