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

React 15.3.1 shouldn't be a dependency. #1386

Closed
salazareddie opened this issue Oct 5, 2016 · 13 comments · Fixed by #1396
Closed

React 15.3.1 shouldn't be a dependency. #1386

salazareddie opened this issue Oct 5, 2016 · 13 comments · Fixed by #1396

Comments

@salazareddie
Copy link

Propose using
"react": "^0.14.0" in devdependencies
See this example

@vvo
Copy link
Contributor

vvo commented Oct 6, 2016

Hi @salazareddie, thanks for the issue. Can you explain and comment how/if the current state is triggering some bad behavior on your side?

Thanks

@salazareddie
Copy link
Author

@vvo

For NPM < 3 this causes two version of react to be loaded, and the app to go kaboom. The guys at material-ui had a similar issue, here.

@vvo
Copy link
Contributor

vvo commented Oct 6, 2016

Yep you're right thanks, it should be a peerDependency thought right? Because if you use this with npm, you will need react to be installed. Thus a peer will indicate this to you.

Also this may break some existing installations of instantsearch.js for npm users if we do this. For sure next breaking version we will do it, but for now we cannot.

What we can do is move to "^15.0.0" instead of "15.3.2" would that be fine for you? So that npm resolves this well.

@salazareddie
Copy link
Author

@vvo Yes, it should be a peerDependency. Moving to ^15.0.0 should work for us. I think you will have to do the same for react-dom. Will grab latest once you push. Thanks for the quick turn around.

@vvo
Copy link
Contributor

vvo commented Oct 6, 2016

Ok so we are gonna move to ^15.0.0 put stick with dependencies field. We will move to peerDependency in another breaking version. None planned yet.

@vvo
Copy link
Contributor

vvo commented Oct 6, 2016

@salazareddie What's the react version you are using right now?

@salazareddie
Copy link
Author

salazareddie commented Oct 6, 2016

@vvo We are using 0.14.8. Tested the above solution locally and will do the trick. Just get these warnings when I npm install:

npm WARN unmet dependency /Users/esalazar/projects/***/node_modules/instantsearch.js requires react@'^15.0.0' but will load
npm WARN unmet dependency /Users/esalazar/projects/***/node_modules/react,
npm WARN unmet dependency which is version 0.14.8
npm WARN unmet dependency /Users/esalazar/projects/***/node_modules/instantsearch.js requires react-dom@'^15.0.0' but will load
npm WARN unmet dependency /Users/esalazar/projects/***/node_modules/react-dom,
npm WARN unmet dependency which is version 0.14.8

@vvo
Copy link
Contributor

vvo commented Oct 6, 2016

ok fine then thx, will do it assap

@salazareddie
Copy link
Author

@vvo any updates on this?

vvo pushed a commit that referenced this issue Oct 7, 2016
Ultimately we should have React as a peerDependency.

fixes #1386
vvo pushed a commit that referenced this issue Oct 7, 2016
Ultimately we should have React as a peerDependency.

fixes #1386
@vvo
Copy link
Contributor

vvo commented Oct 7, 2016

Going to do a release in the coming hour, are you a react user? We do have a react specific set of components in the work right now if you want to beta test it, email at vincent@algolia.com thx

vvo pushed a commit that referenced this issue Oct 7, 2016
Ultimately we should have React as a peerDependency.

fixes #1386
@vvo vvo closed this as completed in #1396 Oct 7, 2016
vvo pushed a commit that referenced this issue Oct 7, 2016
Ultimately we should have React as a peerDependency.

fixes #1386
vvo pushed a commit that referenced this issue Oct 7, 2016
<a name="1.8.9"></a>
## [1.8.9](v1.8.8...v1.8.9) (2016-10-07)

### Bug Fixes

* **react:** avoid duplicating React ([59010f6](59010f6)), closes [#1386](#1386)
@vvo
Copy link
Contributor

vvo commented Oct 7, 2016

should be fixed, let me know

@salazareddie
Copy link
Author

@vvo Do you want me to add a separate issue for moving the dependency to peer dependencies?

@vvo
Copy link
Contributor

vvo commented Oct 7, 2016

@salazareddie It's ok we'll do it but only on a breaking change, should happen this year :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants