Skip to content
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

Handling edge case bugs of create app command #537

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

spenserhale
Copy link

Fixing three bugs:

  1. If your project name has spaces, the command will fail. Now, packageName is used as the folder name. In the future, the project name could be used, but this would require a larger rewrite to support.
  2. npm create vite@latest never ran on node version 20+. Now, it uses the exec command instead of spawn.sync(). Also, it now handles if the command fails and prints the error and exists.
  3. Reading the template package.json sometimes asks for the file before it is fully written to the OS (race condition). Now, it has a new function, readFileWithRetries, to wait for the file to be ready. It also handles if a file is not found by printing an error and exiting.

Other Changes:
Updating CLI defaults to order match preferred, and making CLI version check a one-liner.

Tested on Node 20, 18.

Updating README to have version check command print both node and npm versions in one go.
Currently when you copy command and paste into terminal only prints one.
Updating Constants for Backends and Databases to provide the preferred option as the default
Fixing three bugs:
1) If your project name has spaces, the command will fail. Now uses packageName as folder name.
2) npm create vite@latest never runs on node version 20+. Now uses exec command instead of spawn.sync.
3) Reading the template package.json asks for file before the file is fully written to OS. Now has new function readFileWithRetries to wait for file is ready.
throw new Error(`${filePath} could not be found after ${retries} attempts`);
}

export function execCommandExitOnError(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove the existing runCommand that is using spawn.sync and the cross-spawn import then?

Copy link
Author

Choose a reason for hiding this comment

The 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.

cwd,
true,
)
execCommandExitOnError(`npm create vite@latest "${targetDir}" -- --template react-ts`)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would adding await here save the need to retry reading?

Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants