Skip to content

Commit

Permalink
Move react to peerDependencies
Browse files Browse the repository at this point in the history
Summary:
This PR moves `react` from dependencies to peerDependencies.

In general, this would have only been important for those people using packages that depend on `react` and were using npm@2...npm@3 would automatically de-dupe.

However, when #5812 gets merged, dependencies will be scoped to react-native (on both npm@2 & npm@3), thus breaking projects that are using a package like `react-redux` for example, which depends on `react`. There would be two copies of React installed, and due to the use of haste modules in `react`, this would break the packager and cause naming collisions.

This PR does three things -

1. Moves the dependency from dependencies to peerDependencies
2. Updates the local-cli to run `npm install react --save` when a new project is initialized.
3. Updates `react-native upgrade` to warn if `react` is not listed in the package.json's dependencies.

**Note: This will require a shrinkwrap update.**
Closes #5813

Reviewed By: svcscm

Differential Revision: D2918380

Pulled By: androidtrunkagent

fb-gh-sync-id: 6e4234a45284be2fdf6fedf29e70b2d2d0262486
shipit-source-id: 6e4234a45284be2fdf6fedf29e70b2d2d0262486
  • Loading branch information
skevy authored and facebook-github-bot-1 committed Feb 9, 2016
1 parent d97223b commit 9f01f96
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 1 deletion.
8 changes: 8 additions & 0 deletions local-cli/generator/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,5 +82,13 @@ module.exports = yeoman.generators.NamedBase.extend({
{name: this.name}
);
}
},

install: function() {
if (this.options.upgrade) {
return;
}

this.npmInstall('react', { '--save': true });
}
});
13 changes: 13 additions & 0 deletions local-cli/upgrade/upgrade.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,19 @@ module.exports = function upgrade(args, config) {
'https://github.com/facebook/react-native/releases/tag/v' + semver.major(v) + '.' + semver.minor(v) + '.0'
)
);

// >= v0.21.0, we require react to be a peer depdendency
if (semver.gte(v, '0.21.0') && !pak.dependencies['react']) {
console.log(
chalk.yellow(
'\nYour \'package.json\' file doesn\'t seem to have \'react\' as a dependency.\n' +
'\'react\' was changed from a dependency to a peer dependency in react-native v0.21.0.\n' +
'Therefore, it\'s necessary to include \'react\' in your project\'s dependencies.\n' +
'Just run \'npm install --save react\', then re-run \'react-native upgrade\'.\n'
)
);
return Promise.resolve();
}
} else {
console.log(
chalk.yellow(
Expand Down
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,9 @@
"bin": {
"react-native": "local-cli/wrong-react-native.js"
},
"peerDependencies": {
"react": "^0.14.5"
},
"dependencies": {
"absolute-path": "^0.0.0",
"art": "^0.10.0",
Expand Down Expand Up @@ -133,7 +136,6 @@
"optimist": "^0.6.1",
"progress": "^1.1.8",
"promise": "^7.1.1",
"react": "^0.14.5",
"react-timer-mixin": "^0.13.2",
"react-transform-hmr": "^1.0.2",
"rebound": "^0.0.13",
Expand Down

5 comments on commit 9f01f96

@martinbigio
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skevy I'm running into issues when npm installing react-native from master. The problem is repro-ing because of peer dependencies: the node_modules directory contains both react and react-native and due to the haste modules in fbjs I get lots of naming collisions. The 2 versions of react I end up having downloaded are 0.14.5 and 0.14.7. I get this even if I specify one of this 2 for react on the test project package.json (which sounds like an npm bug).

I'm pretty sure this only repros on npm2 because on npm3 peer dependencies are not automatically downloaded. Have you tested this on npm2? Moving forward should be support both npm2 and npm3 or should be deprecate npm2 support?

This rev is on the RC. Could someone check if I'm not the only one running into this issue? Thanks!

cc @mkonicek, @davidaurelio, @vjeux

@skevy
Copy link
Contributor Author

@skevy skevy commented on 9f01f96 Feb 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martinbigio I can play with this tonight or in the morning. I did test on npm2...but I think specifically this could be a problem with the shrinkwrap. I'm sure we can come up with a solution though.

Re: deprecating npm2 support...i don't think it's necessary. I honestly think that this will cease to be a problem once 5084 is merged. And we're really only encountering this stuff because of Haste and because of our weird in between state that we're in.

I think that most of the issues people have had with npm, duplicate module errors, Babel, etc are all fixed as we continue to remove complexity and introduce more Node/Babel best practices -- moving react to a peerDep, the Babel preset, fixing the fbjs stuff.

As much of a pain as this is right now (and I'm sure we can get the current problems fixed)...it's going to be much better soon.

@ide
Copy link
Contributor

@ide ide commented on 9f01f96 Feb 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Landing the fbjs diff (#5084) should be a priority since it fixes the name collision bug, not just between react and react-native, but also between any other package that uses fbjs.

Regarding the RC I'm leaning towards backing this commit out if we can do so cleanly. It's not 100% useful on its own without #5084 since e.g. Relay depends on fbjs.

@skevy
Copy link
Contributor Author

@skevy skevy commented on 9f01f96 Feb 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ide let me see if I can get this to work easily...if not...we can just back it out (and yah, it should be easy to do)

@martinbigio
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for answering so quickly. I agree that this, among many of the other best practices we're embracing will make all this much better very soon and I'm super thankful you guys are taking point on this!.

Please sign in to comment.