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

Don't crash if users mistakingly install babel, webpack, jest, loaders, etc, at the top level #1795

Closed
gaearon opened this issue Mar 11, 2017 · 28 comments
Milestone

Comments

@gaearon
Copy link
Contributor

gaearon commented Mar 11, 2017

I’ve seen this issue multiple times. For some reason people don’t believe react-scripts is enough and try to install Babel, Webpack, and friends themselves. This can break things.

We need to mitigate this somehow.

@gaearon
Copy link
Contributor Author

gaearon commented Mar 17, 2017

@anyayunli If you’d like control over configs, run npm run eject and you have it. This issue is not related to that.

@langpavel
Copy link

This is about inexperienced developers. It can be limited only by more educational examples and strict rules for opening issues.

@langpavel
Copy link

And more visible dedicated site for users, stack overflow is not enough

@Timer
Copy link
Contributor

Timer commented Mar 17, 2017

Is there anything wrong with us showing a nasty-looking warning when people do this?

@langpavel
Copy link

No, really not. You are experiencing success, you only need way to fastly redirect people from Github to more general discussion about when they should eject!

@Timer
Copy link
Contributor

Timer commented Mar 17, 2017

Optimally, we want people not to eject. I feel like there's way too many small things people eject for that's accomplishable without ejecting (e.g. using sass-loader).

@langpavel
Copy link

So, this can be statistically observed. You can inject some questionary into eject command with helpful links. This is task for @cpojer like person:-)

@Timer
Copy link
Contributor

Timer commented Mar 17, 2017

@gaearon what about something like this?

@langpavel
Copy link

Nice, but not enough. It will works BUT you are not providing hint. When you run eject, it should ask you why you need eject providing most relevant information. No way back after. That's ok, but warn about they are not supported by this awesome toolkit any more..

@gaearon
Copy link
Contributor Author

gaearon commented Mar 17, 2017

I’d like us to first look into why this breaks CRA. If there's a version conflict, react-scripts should use its own version of webpack. We should fix whatever breaks so that outer version of webpack is just ignored. We can optionally warn on top of that but first we should fix the breakage.

@langpavel
Copy link

Different idea. Can you hook yarn with CRA? For example as preinstall, pre-add in package.json. I know, this is something new! But this is in your possibilities!
Or you can create recommended way to install new dependencies with CRA specific cli tool, promote it and drive mainstream. This will give you less observable force than patching yarn. Controversial but, hey, look at real people!

@gaearon
Copy link
Contributor Author

gaearon commented Mar 17, 2017

We already use Yarn when it’s installed. It doesn’t solve the problem though.

@langpavel
Copy link

Yarn hook which will dog watching what you want add

@langpavel
Copy link

It's entire new space not possible today. Yes you can give recommendations only..:
You just installed package xyz. Did you consider this way? Http....

@langpavel
Copy link

Where this recommendations can be stored? In package.json at first, at remote server specified in package, at hashtag on Twitter.. irc is almost dead but why not.. So first class will be custom scripts. In future this will be there..

@GAumala
Copy link
Contributor

GAumala commented Jan 8, 2018

Is there any repo that easily reproduces this issue? I'd like to give it a look...

@gaearon
Copy link
Contributor Author

gaearon commented Jan 8, 2018

No idea, but if you can create one that would be great. My impression is you need to create a project and then install eslint@4, babel@5, jest@16, etc, and try to trigger it this way.

@gaearon
Copy link
Contributor Author

gaearon commented Jan 8, 2018

Here is an example, updating jest to 22 might reproduce this.
jestjs/jest#5119 (comment)

@omeid
Copy link

omeid commented Jan 8, 2018

Any solution to this would require CRA to work around how the package manager works, which in my opinion is not the right thing. A package should not override the package manager rules.

I am okay with a mechanism that detects wrong versions of the packages and warn, or at worst, just give up.

@viankakrisna
Copy link
Contributor

experimented using pkg to package react-scripts as an executable: https://github.com/viankakrisna/react-scripts-packaged/releases/tag/0.0.1

@jlongster
Copy link

I personally think Jest should be excluded from this list. Using testing through CRA is optional. I'd like to use CRA for all the tooling to setup and run and app, but do testing myself (I like to interact with jest directly).

Maybe the create-react-app script to run tests could do this check, so you would only fail if you locally had jest installed and tried to run tests through CRA. Maybe that's how it's intended to work and I missed that point. :) If so, that sounds great.

@gaearon
Copy link
Contributor Author

gaearon commented Jan 9, 2018

I think we keep talking past each other. Sorry if I'm being confusing.

I don't propose to forbid using your own version. (Early wording says that but my more recent comments don't.)

What I'm saying is we should make sure that installing arbitrary Jest/Babel/Whatever version at top level does not break any of built-in react-scripts functionality. So you should be able to use them side by side if you know what you're doing. In fact that's how I'd expect it to already work—I have no idea why it breaks (according to reports).

Does this clarify things? That's the only problem I'm referring to in this issue.

@jlongster
Copy link

I may have missed a reply, especially since there were two threads that got merged. Sounds great. Sorry if I didn't read the original description well enough.

@gaearon
Copy link
Contributor Author

gaearon commented Jan 12, 2018

I did some debugging. The Jest issue is caused by how Jest packages communicate: jestjs/jest#5294.

@gaearon gaearon changed the title People try to install babel, webpack, loaders, etc, at top level Don't crash if users mistakingly install babel, webpack, jest, loaders, etc, at the top level Jan 12, 2018
@gaearon
Copy link
Contributor Author

gaearon commented Jan 12, 2018

For Webpack, this happens (at least with npm) because:

  • node_modules/react-scripts imports node_modules/html-webpack-plugin (which npm hoists at the top for some reason)
  • node_modules/html-webpack-plugin imports webpack and gets the top-level "wrong" node_modules/webpack rather than the intended node_modules/react-scripts/node_modules/webpack

I don't know why html-webpack-plugin gets hoisted but I also don't quite see how to avoid it. For example in monorepos it's expected more things get hoisted.

@gaearon
Copy link
Contributor Author

gaearon commented Jan 13, 2018

I chatted to npm folks about #1795 (comment) — it is a known npm issue. Yarn doesn't have it. They tried to fix it in 5.x but that created other issues so they're holding off until future majors.

@gaearon
Copy link
Contributor Author

gaearon commented Jan 13, 2018

#3771

@gaearon
Copy link
Contributor Author

gaearon commented Jan 13, 2018

Fixed by #3771.

There is a way to opt out of the preflight check for those who know what they're doing (it's mentioned at the end of the displayed message).

@gaearon gaearon closed this as completed Jan 13, 2018
@lock lock bot locked and limited conversation to collaborators Jan 20, 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

7 participants