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

Crash the app if does not find index.html, index.js or favico.ico #653

Closed
wants to merge 5 commits into from

Conversation

vnctaing
Copy link
Contributor

Hi,

I saw that issue(#96) needed help, it had previously a PR that have been closed few weeks back. I don't know if someone is still working this. So I tried to solve the issue

How can I improve my PR ?

Thanks !

@ghost
Copy link

ghost commented Sep 15, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

@ghost ghost added the CLA Signed label Sep 15, 2016
@ghost
Copy link

ghost commented Sep 15, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

Copy link
Contributor

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

Please fix per the comments.

@@ -34,7 +34,9 @@ function resolveApp(relativePath) {
// config after eject: we're in ./config/
module.exports = {
appBuild: resolveApp('build'),
appFavico: resolveApp('src/favicon.ico'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Favicon is not required anymore. We can remove it from the list of files to check.

appHtml: resolveApp('index.html'),
appIndexJs: resolveApp('src/index.js'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Let’s use this variable instead of path.join(paths.appSrc, 'index') in webpack.config.dev and .prod too.

function checkRequiredFiles() {
var filesPathToCheck = [paths.appHtml, paths.appIndexJs, paths.appFavico];
filesPathToCheck.forEach(function(filePath) {
fs.access(filePath, fs.constants.F_OK, function(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is asynchronous so you are going to have race conditions. The code below will execute before these callbacks.

Instead, we should do a synchronous access here.

var filesPathToCheck = [paths.appHtml, paths.appIndexJs, paths.appFavico];
filesPathToCheck.forEach(function(filePath) {
fs.access(filePath, fs.constants.F_OK, function(err) {
if(err) {
Copy link
Contributor

@gaearon gaearon Sep 16, 2016

Choose a reason for hiding this comment

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

Minor nit: please put space after if

@gaearon
Copy link
Contributor

gaearon commented Sep 18, 2016

Cherry-picked via 4a0f39a.

I rebased to tag you as the commit author because your local commit email doesn’t match your GitHub email. You need to change local git settings to use your GitHub email so that in the future, your userpic shows up and your contributions get credited in project history.

Cheers!

@gaearon gaearon closed this Sep 18, 2016
@gaearon gaearon added this to the 0.4.2 milestone Sep 18, 2016
@gaearon gaearon mentioned this pull request Sep 18, 2016
feiqitian pushed a commit to feiqitian/create-react-app that referenced this pull request Oct 25, 2016
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants