halogen icon indicating copy to clipboard operation
halogen copied to clipboard

Update to react 15

Open abhilashsajeev opened this issue 8 years ago • 19 comments

@yuanyan Removed peer dependencies and added react as dependencies.

abhilashsajeev avatar Apr 13 '16 07:04 abhilashsajeev

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

idris avatar Apr 25 '16 19:04 idris

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 avatar Apr 27 '16 22:04 jharris4

@jharris4 Why peerDependency is good for React 16?

yuanyan avatar May 01 '16 11:05 yuanyan

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)

jharris4 avatar May 01 '16 16:05 jharris4

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

Merging this PR will be helpful

abhilashsajeev avatar May 02 '16 04:05 abhilashsajeev

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

jharris4 avatar May 02 '16 16:05 jharris4

@yuanyan @jharris4 Added react as both peerDependency and dependency

abhilashsajeev avatar May 05 '16 11:05 abhilashsajeev

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.

jharris4 avatar May 05 '16 12:05 jharris4

@jharris4 @yuanyan Done :+1:

@yuanyan Hope you can merge this now.

abhilashsajeev avatar May 05 '16 12:05 abhilashsajeev

When will this be merged?

pke avatar Jun 08 '16 14:06 pke

@yuanyan Any update?

abhilashsajeev avatar Jun 08 '16 14:06 abhilashsajeev

Request to get this change merged upstream

jacobdr avatar Jun 27 '16 15:06 jacobdr

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

pke avatar Jun 27 '16 15:06 pke

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?

jacobdr avatar Jun 27 '16 15:06 jacobdr

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.

pke avatar Jun 27 '16 15:06 pke

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

abhilashsajeev avatar Jun 28 '16 03:06 abhilashsajeev

Blocking us from using atm

natew avatar Jul 28 '16 20:07 natew

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

jharris4 avatar Jul 28 '16 20:07 jharris4

Can you merge this please? @yuanyan

eyarham avatar Mar 22 '17 16:03 eyarham