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

.babelrc and eject script #1806

Closed
dbismut opened this issue Mar 13, 2017 · 27 comments
Closed

.babelrc and eject script #1806

dbismut opened this issue Mar 13, 2017 · 27 comments

Comments

@dbismut
Copy link
Contributor

dbismut commented Mar 13, 2017

Hey - I'm very new (or at least I'm far from understanding everything) but working on a fork of this project using inferno, I'm running into an issue with the eject script, namely that the .babelrc file can't be found.

Looking at the code of react-scripts and specifically this line, it looks like the eject script looks for a file called .babelrc (notice the dot).

During this commit .babelrc was renamed to babelrc.

I'm not running into bugs with create-react-app, but I was wondering if everything was normal?

@gaearon
Copy link
Contributor

gaearon commented Mar 13, 2017

Good catch. I don’t understand why end-to-end test passes on master. 😳

This doesn’t affect our users because they’re on 0.9.x branch. That’s where stable releases are, until 0.10 is ready. I would not recommend anyone to use code in master of this repo right now.

@gaearon gaearon added this to the 0.10.0 milestone Mar 13, 2017
@gaearon
Copy link
Contributor

gaearon commented Mar 13, 2017

(The fix would be to hardcode Babel and ESLint "configs" since they're one liners.)

@gaearon
Copy link
Contributor

gaearon commented Mar 13, 2017

This is showing up in Travis logs:

(node:3029) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: ENOENT: no such file or directory, open '/tmp/tmp.F2nYJH2DAJ/test-app/node_modules/react-scripts/.babelrc'
(node:3029) DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

We should somehow enable this behavior sooner in end-to-end test.

And I don't understand how the project still builds after ejecting on CI without those fields..

@gaearon
Copy link
Contributor

gaearon commented Mar 13, 2017

OK, this is embarrassing.

screen shot 2017-03-13 at 3 35 49 pm

Our end-to-end test doesn’t verify that ejection actually happened. So we get an unhandled Promise rejection, ejecting stops, but since it doesn’t exit with an error, the project is still in half-working state, and so it passes the rest of the end-to-end test.

If you’d like to help, please send a PR that:

  1. Makes the end-to-end tests (files in tasks/*.sh) crash on unhandled rejection. Since you probably can’t enforce that from a shell file, maybe we should just do that in every scripts/*.js file. I’m not sure how to opt into this though.

  2. Verify that a crash like this causes eject to exit with non-zero code. This way we can catch ejection errors in end-to-end tests instead of ignoring them.

  3. Fix the actual issue here. This would mean replacing reads from those files and further assignments with something simple like

appPackage.babel = {presets: 'react-app'};
appPackage.eslintConfig = {extends: 'react-app'};

@dbismut
Copy link
Contributor Author

dbismut commented Mar 13, 2017

Hey, thanks for investigating so quickly! I'm not sure I'm the right man for this since I've never done tests in my life (I know this is wrong, but I'll have a look and make up for it), but I was curious on point 3 why we should not read from the file and just change .babelrc to babelrc?

Unless there is absolutely no reason in the future to add other lines to babelrc and eslintrc?

@Timer
Copy link
Contributor

Timer commented Mar 13, 2017

@dbismut you can do it! We're here to help. 😄

As for point three, we simply use these files for the ejection process -- they're not used internally by react-scripts (probably why renamed, to not make devs confused when they edit them and it doesn't change anything).
We can get rid of these because they're only used in one location.


Looks like throwing on an unhandled rejection will be the default behavior in Node 8.

For now, looks like we can opt-in like so:

process.on('unhandledRejection', err => {
  throw err;
});

We should probably add this to scripts/*.sh, not tasks/*.sh since it will become the new default.

A PR implementing 1 and 2 would be a great start (to see an actual failure), we can talk about 3 then.

Thanks so much!

@gaearon
Copy link
Contributor

gaearon commented Mar 13, 2017

@dbismut Can you verify whether the fix in #1810 helps?

@dbismut
Copy link
Contributor Author

dbismut commented Mar 13, 2017

@gaearon It does fix it!

@Timer
Copy link
Contributor

Timer commented Mar 13, 2017

@dbismut did you want to send a PR adding the unhandled rejection throwing to scripts?

@dbismut
Copy link
Contributor Author

dbismut commented Mar 13, 2017

@Timer I would have loved to and currently looking into it, but by the time I understand how all this work, I'm afraid you'd be losing patience. I just figured out how to run tests from the tasks folder (not that I've been spending my whole time at this but still)...

@Timer
Copy link
Contributor

Timer commented Mar 13, 2017

Oh no, nothing like that. I just wasn't sure if you wanted to do it. Sorry if I came off that way.
Take your time. 😄 All yours!

@dbismut
Copy link
Contributor Author

dbismut commented Mar 13, 2017

Haha no worries... I want to try, but I'll let you know if I don't think I can make it by EOD tomorrow.

@dbismut
Copy link
Contributor Author

dbismut commented Mar 13, 2017

@Timer it looks like the below doesn't catch the error.

process.on('unhandledRejection', err => {
  throw err;
});

@gaearon
Copy link
Contributor

gaearon commented Mar 13, 2017

Does something like this work?

process.on('unhandledRejection', err => {
  console.log(err);
  process.exit(1);
});

@dbismut
Copy link
Contributor Author

dbismut commented Mar 13, 2017

Forget about what I said. What @Timer wrote actually works 🙃

@dbismut
Copy link
Contributor Author

dbismut commented Mar 13, 2017

@Timer @gaearon it looks like that in e2e-simple.sh, from line 249, npm run eject doesn't run the script in packages/react-scripts/eject.js. If I move the npm link 252-255 lines before line 249 it actually works.

Is this another bug or just me being dumb again?

@Timer
Copy link
Contributor

Timer commented Mar 13, 2017

Hm, it should work because we run the installer from a packed copy.
See https://github.com/facebookincubator/create-react-app/blob/master/tasks/e2e-simple.sh#L128-L130 and https://github.com/facebookincubator/create-react-app/blob/master/tasks/e2e-simple.sh#L138-L143.

Is it exhibiting behavior that this doesn't happen? If so, that's interesting!
Are you using yarn? Can you try to clean your yarn cache?

@dbismut
Copy link
Contributor Author

dbismut commented Mar 13, 2017

I'm off my desk but will try this tomorrow morning GMT. Yes I'm indeed using yarn. All I can say for now is that the code I modified in the eject.js file wasn't executed in the test!

Anyway thanks for your help, I've learned a lot today ;)

@dbismut
Copy link
Contributor Author

dbismut commented Mar 14, 2017

@Timer Indeed you're right, running yarn cache clean solves it!

Here is the output of tasks/e2e-simple.sh after adding the unhandledRejection event to eject.js. This is when running eject.js with the wrong reading of .babelrc / .eslintrc (which is solved by #1810).

eject

So I guess it solves @gaearon's point number 2?

I have 3 questions then:

  1. Should I add the unhandledRejection to all scripts from react-scripts?
  2. @Timer you mentioned moving the tasks/*.sh to scripts/*.sh: do you want me to do this?
  3. Forgive my ignorance but I have to ask: since Fixes a silent crash when ejecting #1810 modifies eject.js, can I work off the master branch or should I wait for the Fixes a silent crash when ejecting #1810 to be merged first?

Thanks!

@Timer
Copy link
Contributor

Timer commented Mar 14, 2017

  1. Yes, please add this to all scripts.
  2. No. This was my mistake, I meant scripts/*.js.
  3. You should be able to make your edits before Fixes a silent crash when ejecting #1810 is merged. Git is really good at combining things. 👍

Thanks for being on top of this!

@dbismut
Copy link
Contributor Author

dbismut commented Mar 14, 2017

@Timer done! However I re-ran into the yarn cache clean issue, I was wondering if cleaning yarn's cache shouldn't be part of the testing shell scripts?

@Timer
Copy link
Contributor

Timer commented Mar 14, 2017

This is a known Yarn bug (it will be fixed), see yarnpkg/yarn#2649 and yarnpkg/yarn#2165. I thought we clean the cache in one of them ...

@dbismut
Copy link
Contributor Author

dbismut commented Mar 14, 2017

You do in all actually. Not sure why I ran into this issue then...

@Timer
Copy link
Contributor

Timer commented Mar 14, 2017

Ah, you need to have USE_YARN set to yes. This makes sense as these tests are meant for build servers, not so much local testing.

@dbismut
Copy link
Contributor Author

dbismut commented Mar 15, 2017

Should be fixed by #1810 and subsequently by #1819.

@dbismut dbismut closed this as completed Mar 15, 2017
@gaearon
Copy link
Contributor

gaearon commented Mar 15, 2017

Thanks @dbismut!

@dbismut
Copy link
Contributor Author

dbismut commented Mar 15, 2017

My pleasure, thanks for the help and patience, I just followed instructions 💂

@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

No branches or pull requests

3 participants