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

Update to react 15 #24

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

abhilashsajeev
Copy link
Contributor

@abhilashsajeev abhilashsajeev commented Apr 13, 2016

@yuanyan Removed peer dependencies and added react as dependencies.

@idris
Copy link

idris commented Apr 25, 2016

@yuanyan can you please merge this? It's breaking our CI

@jharris4
Copy link

Please avoid adding react as a dependency instead of a peerDependency. Doing so will mean that React 15.x will always be installed when this package is installed which will be bad if/when React 16 comes out.

Much better to keep React as both a peerDependency and a devDependency, that way things won't actually break for people using future versions of React before this package gets updated.

@jharris4 jharris4 mentioned this pull request Apr 27, 2016
@yuanyan
Copy link
Owner

yuanyan commented May 1, 2016

@jharris4 Why peerDependency is good for React 16?

@jharris4
Copy link

jharris4 commented May 1, 2016

If React 15 is a dependency of Halogen and someone installs Halogen in a project that has a dependency on React 16, then both versions of React will be installed which will cause an error.

If React 15 is a peerDependency of Halogen and someone installs Halogen in a project that has a dependency on React 16, then npm will give a warning, but the project will still work correctly if React 15 and React 16 are compatible.

You can see that other packages are following the convention of specifying React as a peerDependency mostly for this reason. For example, see react-motion (https://github.com/chenglou/react-motion/blob/master/package.json) or react-dimensions (https://github.com/digidem/react-dimensions/blob/master/package.json)

@abhilashsajeev
Copy link
Contributor Author

abhilashsajeev commented May 2, 2016

@jharris4 @yuanyan Keeping as peer dependency may break for npm 3+ later versions.

Merging this PR will be helpful

@jharris4
Copy link

jharris4 commented May 2, 2016

No it won't break, that's why you add as BOTH a peerDependency AND a devDependency.

@abhilashsajeev
Copy link
Contributor Author

@yuanyan @jharris4 Added react as both peerDependency and dependency

@jharris4
Copy link

jharris4 commented May 5, 2016

If you add it as a dependency (instead of devDependency) then whichever version of react you list in the dependencies section will always be installed whenever halogen is installed, and the peerDependency is pointless.

The best way to do it would be like this (Note the use of ^ as well to allow newer versions as well):

"peerDependencies": {
  "react": "^0.14 || ^15.0.0"
},
"devDependencies": {
  "react": "^15.0.1",
  "react-dom": "^15.0.1"
}

You can look at the links to the package.json for the projects I pasted above, or many other examples on npm/github to see that this is how most people are managing dependencies on React to make it easier for people to migrate from one version of React to another.

@abhilashsajeev
Copy link
Contributor Author

abhilashsajeev commented May 5, 2016

@jharris4 @yuanyan Done 👍

@yuanyan Hope you can merge this now.

@pke
Copy link
Contributor

pke commented Jun 8, 2016

When will this be merged?

@abhilashsajeev
Copy link
Contributor Author

@yuanyan Any update?

@jacobdr
Copy link

jacobdr commented Jun 27, 2016

Request to get this change merged upstream

@pke
Copy link
Contributor

pke commented Jun 27, 2016

whats with the 2nd commit? It looks like it does not belong here.

@jacobdr
Copy link

jacobdr commented Jun 27, 2016

Perhaps unnecessary, but it does reflect the real history of the changes. I would say look at the discussion on the various approaches in this SO article.

Would you suggest the squash alternative instead?

@pke
Copy link
Contributor

pke commented Jun 27, 2016

It's unfortunate that the owner decided to save generated code in the repo. That makes one file changes (packages.json) to be such mess. But be it as it may, the PR is ok then.
Let see when the owner will finally merge it.

@abhilashsajeev
Copy link
Contributor Author

@jacobdr I did not squash commits to get the whole history.
Also github have already an option to squash and merge.

@natew
Copy link

natew commented Jul 28, 2016

Blocking us from using atm

@jharris4
Copy link

fwiw, I switched to this: https://github.com/jonjaques/react-loaders and it's working well and looks indentical.

@eyarham
Copy link

eyarham commented Mar 22, 2017

Can you merge this please? @yuanyan

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

Successfully merging this pull request may close these issues.

9 participants