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

Allow creating app in existing empty dir #315

Merged
merged 2 commits into from
Mar 24, 2020
Merged

Conversation

olance
Copy link
Contributor

@olance olance commented Mar 22, 2020

I tried yarn create redwood-app . in an empty directory where I wanted the files generated, and got an error saying the dir already exists.
I think it should be OK to use an existing directory if it's empty 🙂

I have also moved the error out of the Listr task itself, because it seems Listr is not actually stopping execution of the following tasks. At least, it continues outputting the next tasks' titles, which is confusing:
image

I'd say an early exit for this particular error is acceptable?

I tried `yarn create redwood-app .` in an empty directory where I wanted the files generated, and got an error saying the dir already exists.  
I think it should be OK to use an existing directory if it's empty 🙂 

I have also moved the error out of the Listr task itself, because it seems Listr is not actually stopping execution of the following tasks. At least, it continues outputting the next tasks' titles, which is confusing:

I'd say an early exit for this particular error is acceptable?
@satyarohith
Copy link
Contributor

This doesn't disturb the flow of a developer, so it's a useful feature to have. Thanks, @olance!

Copy link
Contributor

@peterp peterp left a comment

Choose a reason for hiding this comment

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

Love it! Thank you!

I think we should handle the uncaught exception and then this is ready for merge.

const appDirExists = fs.existsSync(newAppDir)

if (appDirExists && fs.readdirSync(newAppDir).length > 0) {
throw new Error(`'${newAppDir}' already exists and is not empty.`)
Copy link
Contributor

Choose a reason for hiding this comment

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

This would result in an uncaught exception, it would be better to use a combination of console.log and process.exit(1)

@olance
Copy link
Contributor Author

olance commented Mar 22, 2020

@peterp thanks, I’ll do that tomorrow 🙂

@olance olance requested a review from peterp March 23, 2020 09:39
@peterp peterp merged commit 583ca40 into redwoodjs:master Mar 24, 2020
@olance olance deleted the patch-1 branch March 25, 2020 22:31
mohsen1 pushed a commit to mohsen1/redwood that referenced this pull request Apr 12, 2020
Allow creating app in existing empty dir
@thedavidprice thedavidprice added this to the unassigned-version milestone May 14, 2021
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.

4 participants