Gracefully handle initial installation error (#1505)

[chitchu]

As per #1505

Currently, when npm fails for the first time, you end up with an empty project that doesn't work.

We should delete the broken files we added (or the whole folder if we created it), and print a message explaining the problem is with npm, as well as the underlying message.

• Print out message when problem occurs
• Delete project folder on errors

[gaearon]

Is this safe? There are cases when we allow installing into an existing directory if it only contains files we wouldn't clash with. I think this may actually remove user files in this case.

[chitchu]

There are cases when we allow installing into an existing directory

Is there a switch to force that? I tested it and it wouldn't allow me even if the folder contained unrelated files to the project.

foosbar/
└── unrelatedfile.txt

0 directories, 1 file

The directory foosbar contains files that could conflict.
Try using a new directory name.

[Timer]

It only allows a select few files, see https://github.com/facebookincubator/create-react-app/blob/master/packages/create-react-app/index.js#L276.

[chitchu]

Ok got it. Thanks.

[chitchu]

Would it make sense to create a timestamp upon running create-react-app. Then when installation fails the script would only delete the files in the folder that was created after the timestamp?

[gaearon]

Still kinda dangerous. It could run for a long time and user might have created something by hand.

[chitchu]

You're right. Some use case could be a script that creates the project folder, initialises git then runs create-react-app. All files had the same birthtime.

So far I've only seen 3 4 possible files/directory in the project folder when installation fails

• package.json
• npm-debug.log
• yarn-error.log
• node_modules (only gets created if installation is successful)

We could check if any of this files exist, delete them, check if project folder is empty then remove it?

[chitchu]

I've amended the approach. Should be somewhat safer. Still doesn't account for the "user created a file in the folder during installation" use case.

[Timer]

@fson, could you please review this? I would greatly appreciate it.

[Timer]

@chitchu could you please rebase this?

[Timer]

/cc @gaearon

Still doesn't account for the "user created a file in the folder during installation" use case.

Is this an acceptable trade-off? We could always change this to prompt the user.

The app creation failed. Would you like to delete the attempt?
This is a destructive action. You will lose any work attempted during the creation phase. (Y/n)

[chitchu]

The prompt sounds like a good idea. Only prompt if installation failed on an existing folder scenario?

[Timer]

Yeah, I'd only prompt on an install fail + existing folder.

[gaearon]

Hmm, I don't think the prompt is necessary.
I'm not sure I fully understand the problem.

We should always delete everything we created. If, after that, the directory is empty, we should remove the directory as well. Otherwise, we can leave it.

The goal here is that, regardless of whether the directory exists or not, you should not have any files from the broken state after the tool exits, and you should be able to run create-react-app with the same directory again.

[Timer]

I suppose the problem here is that we don't keep a list of files that the installer creates.
This works by deleting all files not on the permitted list pre-install. We should probably switch it but then we need to update this if we ever add or change the file names in the future.

[gaearon]

If npm install (or yarn) fails, then we can only have package.json, node_modules, and their logs, right?

[Timer]

That may be true, I haven't checked. Have we moved the template over yet at that point?

[gaearon]

The template exists inside react-scripts so I don't think we could have.

[Timer]

@chitchu can you switch it to functioning how @gaearon suggested? Delete known files which will be artifacts, then delete the whole folder if it's empty.

[gaearon]

Looks like updating this is the last blocker to 0.9.1 🎉

[chitchu]

Alrighty, amended the approach to only delete known generated files and delete the folder if it's empty.

[Timer]

🔥 LGTM, will test soon.

[gaearon]

#1507 (comment)

[Timer]

I thought about this, but I haven't been able to reproduce this behavior.
... and I assume it only happens for collision-avoidance. I don't think it'd ever happen during an install. + newer versions of npm write to a cache directory instead of the working directory.

Possibly worth handling.

[Timer]

I believe globbing would add additional complexity, and/or an extra dep, so I want to make sure it's necessary before we do it.

@chitchu can you see how hard this would be?

[gaearon]

Ok let's not bother for now.

[chitchu]

I was actually thinking something like

      var knownGeneratedFiles = [
        'package.json', 'npm-debug.log', 'yarn-error.log', 'yarn-debug.log', 'node_modules'
      ];

      var currentFiles = fs.readdirSync(path.join(root));
      currentFiles.forEach(function (file) {
        knownGeneratedFiles.forEach(function (fileToMatch) {
          if (!!~file.indexOf(fileToMatch)) {
            console.log('Deleting generated file...', chalk.cyan(file));
            fs.removeSync(path.join(root, file));
          }
        });
      });

This will catch *npm-debug.log* files.

[Timer]

I don't think it's well known that !!~(-1) === false, I think that !== -1 is more explicit. 😄
If it's this easy, I guess there's no issue. Sorry, I'm really tired and trying to overthink things.

We should probably only apply this to the log files though, not everything. The only scenario where this would malfunction is if someone made a package.json.temp or similar during install -- an edge case for sure ... but we still shouldn't delete the file[s].

[chitchu]

Done! This will delete *(npm-debug|yarn-error|yarn-debug).log* along with package.json and node_modules folder.

[Timer]

We might want this to be file.indexOf(fileToMatch) === 0 so that it only globs the end of the file. Details though, nbd.

Anyway, I've got to head to bed. Goodnight from the east coast!

[Timer]

I just left one more [silly] comment. Thanks so much for being so responsive!
We really appreciate what you're doing. Really.

[chitchu]

Thanks so much for being so responsive!

Well when I saw

Looks like updating this is the last blocker to 0.9.1 🎉

I knew I had to step it up!

[Timer]

Looks like there's a bug in Yarn, but this is working:

+ yarn cache clean
yarn cache v0.20.3
success Cleared cache.
✨  Done in 0.06s.
+ cd /Users/joe/Documents/Development/OSS/create-react-app
+ node packages/create-react-app/index.js --scripts-version=/Users/joe/Documents/Development/OSS/create-react-app/packages/react-scripts/react-scripts-0.9.0.tgz /Users/joe/Desktop/test1
Creating a new React app in /Users/joe/Desktop/test1.

Installing packages. This might take a couple minutes.
Installing react, react-dom, and react-scripts...

yarn add v0.20.3
info No lockfile found.
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
error An unexpected error occurred: "ENOENT: no such file or directory, lstat '/Users/joe/Library/Caches/Yarn/react-scripts'".
info If you think this is a bug, please open a bug report with the information provided in "/Users/joe/Desktop/test1/yarn-error.log".
info Visit https://yarnpkg.com/en/docs/cli/add for documentation about this command.

Aborting installation. yarnpkg add --exact react react-dom /Users/joe/Documents/Development/OSS/create-react-app/packages/react-scripts/react-scripts-0.9.0.tgz has failed.
Deleting generated file... package.json
Deleting generated file... yarn-error.log
Deleting test1 from /Users/joe/Documents/Development/OSS/create-react-app
Done.
++ set +x
cra.sh: ERROR! An error was encountered executing line 83.
Cleaning up.
Exiting with error.
error Command failed with exit code 1.

And with during-install modifications:

Joes-MacBook-Pro:Desktop joe$ touch ~/Desktop/test1/npm-debug.log124125f
Joes-MacBook-Pro:Desktop joe$ touch ~/Desktop/test1/stop.txt

Creating a new React app in /Users/joe/Desktop/test1.

Installing packages. This might take a couple minutes.
Installing react, react-dom, and react-scripts...

yarn add v0.20.3
info No lockfile found.
[1/4] 🔍  Resolving packages...
error An unexpected error occurred: "https://registry.yarnpkg.com/destroy: read ETIMEDOUT".
info If you think this is a bug, please open a bug report with the information provided in "/Users/joe/Desktop/test1/yarn-error.log".
info Visit https://yarnpkg.com/en/docs/cli/add for documentation about this command.

Aborting installation. yarnpkg add --exact react react-dom /Users/joe/Documents/Development/OSS/create-react-app/packages/react-scripts/react-scripts-0.9.0.tgz has failed.
Deleting generated file... npm-debug.log124125f
Deleting generated file... package.json
Deleting generated file... yarn-error.log
Done.

Joes-MacBook-Pro:Desktop joe$ ls ~/Desktop/test1/
stop.txt

[Timer]

Only problem I see is that Deleting test1 from /Users/joe/Documents/Development/OSS/create-react-app is wrong.
I believe this should be root.

[Timer]

I went ahead and changed it since it was trivial, thanks so much @chitchu!

diff --git a/packages/create-react-app/index.js b/packages/create-react-app/index.js
index aa0dd33..d5afee4 100755
--- a/packages/create-react-app/index.js
+++ b/packages/create-react-app/index.js
@@ -202,7 +202,7 @@ function run(root, appName, version, verbose, originalDirectory, template) {
       var remainingFiles = fs.readdirSync(path.join(root));
       if (!remainingFiles.length) {
         // Delete target folder if empty
-        console.log('Deleting', chalk.cyan(appName), 'from', chalk.cyan(originalDirectory));
+        console.log('Deleting', chalk.cyan(appName + '/'), 'from', chalk.cyan(path.resolve(root, '..')));
         fs.removeSync(path.join(root));
       }
       console.log('Done.');

yielding

Aborting installation. yarnpkg add --exact react react-dom /Users/joe/Documents/Development/OSS/create-react-app/packages/react-scripts/react-scripts-0.9.0.tgz has failed.
Deleting generated file... package.json
Deleting generated file... yarn-error.log
Deleting test1/ from /Users/joe/Desktop
Done.

[Timer]

🎉