-
Notifications
You must be signed in to change notification settings - Fork 6
Handling edge case bugs of create app command #537
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import fs from "node:fs" | ||
| import path from "node:path" | ||
| import spawn from "cross-spawn" | ||
| import { exec } from "child_process" | ||
|
|
||
| const renameFiles: Record<string, string> = { | ||
| _gitignore: ".gitignore", | ||
|
|
@@ -20,6 +21,34 @@ export async function replaceStringInFile( | |
| ) | ||
| } | ||
|
|
||
| export async function readFileWithRetries(filePath: string, retries: number = 5, delay: number = 1000): Promise<string> { | ||
| for (let attempt = 1; attempt <= retries; attempt++) { | ||
| if (fs.existsSync(filePath)) { | ||
| return await fs.promises.readFile(filePath, "utf-8"); | ||
| } | ||
| await new Promise(resolve => setTimeout(resolve, attempt * delay)); | ||
| } | ||
| throw new Error(`${filePath} could not be found after ${retries} attempts`); | ||
| } | ||
|
|
||
| export function execCommandExitOnError( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we remove the existing
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did the exec since it spawns a new process, as there might be a bug with spawn.sync because it tries to reuse the same shell. It is the reason I believe 'npm create vite@latest' was failing, but is more of a hunch than 100% confirmed. I haven't tested on Windows, so likely the better approach would be to see if cross-spawn is needed for Windows support and then decide if the project can just use the simpler standard library function or if the third-party library is needed, which in that case, the execCommandExitOnError should be rewritten to use cross-spawn for compatibility. |
||
| command: string, | ||
| args: string[] = [], | ||
| ): void { | ||
| exec(`${command} ${args.join(" ")}`, | ||
| (error, stdout, stderr) => { | ||
| if (error) { | ||
| console.error(`${command} error: ${error.message}`) | ||
| process.exit(1) | ||
| } | ||
| if (stderr) { | ||
| console.error(`${command} stderr: ${stderr}`) | ||
| process.exit(1) | ||
| } | ||
| }, | ||
| ) | ||
| } | ||
|
|
||
| export function runCommand( | ||
| fullCommand: string, | ||
| cwd: string, | ||
|
|
||
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.
Would adding
awaithere save the need to retry reading?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.
You might be able to code with a then or a delay, but the core reason for retry is to avoid any I/O delays with the OS that could happen when the node asks for a file but the OS has not fully registered filesystem changes. For example, if you are working within a virtual or symlinked folder, there can be a delay between a written file and when you can read it, and we want to handle edge cases to make the program more robust.