yuanyan / halogen

A collection of loading spinners with React.js
https://yuanyan.github.io/halogen
MIT License
1.6k stars 151 forks source link

Update to react 15 #24

Open abhilashsajeev opened 8 years ago

abhilashsajeev commented 8 years ago

@yuanyan Removed peer dependencies and added react as dependencies.

idris commented 8 years ago

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

jharris4 commented 8 years ago

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.

yuanyan commented 8 years ago

@jharris4 Why peerDependency is good for React 16?

jharris4 commented 8 years ago

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 commented 8 years ago

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

Merging this PR will be helpful

jharris4 commented 8 years ago

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

abhilashsajeev commented 8 years ago

@yuanyan @jharris4 Added react as both peerDependency and dependency

jharris4 commented 8 years ago

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 commented 8 years ago

@jharris4 @yuanyan Done :+1:

@yuanyan Hope you can merge this now.

pke commented 8 years ago

When will this be merged?

abhilashsajeev commented 8 years ago

@yuanyan Any update?

jacobdr commented 8 years ago

Request to get this change merged upstream

pke commented 8 years ago

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

jacobdr commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

natew commented 8 years ago

Blocking us from using atm

jharris4 commented 8 years ago

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

eyarham commented 7 years ago

Can you merge this please? @yuanyan