Move `react` to peerDependencies #5813

skevy · 2016-02-08T05:27:32Z

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 [email protected]@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 -

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

Note: This will require a shrinkwrap update.

facebook-github-bot · 2016-02-08T05:28:30Z

By analyzing the blame information on this pull request, we identified @mkonicek, @frantic and @winterbe to be potential reviewers.

skevy · 2016-02-08T05:29:54Z

cc @ide

ide · 2016-02-08T05:50:50Z

local-cli/upgrade/upgrade.js

+          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' +


style suggestion: I often use template strings for messages so quotes don't have to be escaped

@ide I was just matching other places in the file. I guess I could update the whole file...are template strings supported everywhere now? (the local-cli isn't run through babel-register)

IIRC they shipped with Node 4

facebook-github-bot · 2016-02-08T06:51:11Z

@skevy updated the pull request.

facebook-github-bot · 2016-02-08T06:52:14Z

@skevy updated the pull request.

mkonicek · 2016-02-08T15:44:12Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "react-native",
-  "version": "0.0.0-master",
+  "version": "0.22.0",


I think we want to keep the version here to make it clear we're on master?

This was a remnant of testing in the middle of the night and me being tired. Will fix. :)

mkonicek · 2016-02-08T15:49:31Z

Updates react-native-cli to install react ... this will require a release of a new version of rn CLI

Is this still relevant? The CLI is here: https://github.com/facebook/react-native/blob/master/react-native-cli/index.js

What happens when people don't run react-native upgrade and simply bump the version of react-native in the package.json of their apps?

mkonicek · 2016-02-08T15:50:24Z

local-cli/generator/index.js

+      return;
+    }
+
+    this.npmInstall('react', { '--save': true });


Does this respect the declared version of React via peerDependencies (0.14.5)?

very good question, might be worth installing the version we know matches react-native's one, we can get it from RN's package.json's peerDependencies.

+1 to reading the version from RN package.json

mkonicek · 2016-02-08T15:50:41Z

I'm not the best person to review this. cc @martinbigio

skevy · 2016-02-08T17:27:53Z

@mkonicek that note is not still relevant...I took @frantic's advice and moved the code that does anything re: installing react-native to local-cli. PR description has been updated accordingly.

…project's dependencies when the react-native version is >= 0.21.0.

facebook-github-bot · 2016-02-08T17:32:17Z

@skevy updated the pull request.

mkonicek · 2016-02-09T17:49:17Z

Cool, thanks @skevy! I'll let @martinbigio or @ide merge this as I'm not that familiar with npm.

ide · 2016-02-09T19:06:03Z

I restarted the E2E tests since the failure looked related to the npm repo. Let's get the E2E tests passing and then merge this. We have a bunch of time including a 2-week RC period to shake out issues.

ide · 2016-02-09T19:52:16Z

E2E tests look good. I took a closer look at the Obj-C tests and found that they run the UIExplorer. So @skevy I think we either need to add "npm install react" to travis.yml (hacky solution but has some nice properties, like we install all of RN's dependencies just once) or set up a package.json in each example project and be sure to run something like cd Examples/UIExplorer && npm install in the Obj-C test

skevy · 2016-02-09T20:04:59Z

@ide I don't think the obj-c test failed because of this change.

It's still installing react:

That's because npm only doesn't install peer deps automatically when it's installing dependencies. When you run npm install in the root of a project, like React Native, it will install dev/prod/peer deps.

ide · 2016-02-09T21:39:25Z

Snazzy. move fast break things, let's do it

ide · 2016-02-09T21:39:29Z

@facebook-github-bot shipit

facebook-github-bot · 2016-02-09T21:40:30Z

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1964504793775223/int_phab to review.

facebook-github-bot · 2016-02-09T21:48:15Z

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1964504793775223/int_phab to review.

skevy · 2016-02-09T21:55:43Z

Excuse me @ide I think it's "move fast with STABLE INFRA".

Get it right.

skevy · 2016-02-09T21:57:20Z

In all seriousness though, somebody (@mkonicek?) stop this import. I didn't fix the npm install command in the generator, and would like to do that before this goes to core.

skevy · 2016-02-09T21:59:49Z

Eh. nvm. I'll just fix in a follow up diff. This works for now.

ide · 2016-02-09T22:02:28Z

This will get Relay working. We can fix things like getting the react version right shortly after.

skevy · 2016-02-09T22:05:47Z

*almost will get Relay working...still waiting on #5084 :(

mkonicek · 2016-02-10T14:33:47Z

Note: This will require a shrinkwrap update.

Can you elaborate please? @davidaurelio do we need to do anything internally?

Snazzy. move fast break things, let's do it

@skevy Are you happy with the PR now? Is there a followup we should merge?

mkonicek · 2016-02-10T14:38:03Z

@skevy I'm trying to understand the note about the shrinkwrap file - why not update it as part of this PR? https://github.com/facebook/react-native/blob/master/npm-shrinkwrap.json

skevy · 2016-02-10T15:12:21Z

@mkonicek honestly I didn't update the shrinkwrap because I thought that you guys had a process internally that would update the shrinkwrap automatically when deps were changed. I'm not sure where I heard that :)

If you don't mind updating it for this change (given that this is already merged in)...and then in the future any PR's I send that change dependencies, I'll make sure I update shrinkwrap.

ide · 2016-02-10T20:44:11Z

The issue with updating shrinkwrap.json (as I understand it) is that FB might not have the correct versions of the modules in trunk, so the shrinkwrap had to be generated by FB.

This might be addressed recently by the FB npm package tools. Ideally what would happen is PRs would include npm-shrinkwrap.json, then something inside of FB would run npm install and get all of the packages, and then feed the packages into FB's npm tooling. That way npm-shrinkwrap.json acts as a source of truth.

davidaurelio · 2016-02-15T13:57:01Z

This breaks our internal shrinkwrap process @mkonicek @bestander @martinbigio

(when using npm3. npm2 is fine)

mkonicek · 2016-03-03T16:59:38Z

@skevy I just created a new app with npm2. package.json has deps as follows:

"dependencies": {
  "react": "^0.14.7",
  "react-native": "^0.22.0"
}

I'm using npm2. When starting the packager using npm start I get:

Failed to build DependencyGraph: @providesModule naming collision:
  Duplicate module name: ViewportMetrics
 Paths: /private/tmp/Zero22/node_modules/react/lib/ViewportMetrics.js collides with /private/tmp/Zero22/node_modules/react-native/node_modules/react/lib/ViewportMetrics.js

So it looks like this PR fixed collisions for some use cases but broke them for others (specifically, no one is able to work on their React Native apps if they're using npm 2).

mkonicek · 2016-03-03T17:04:23Z

@skevy tells me this is resolved by facebookarchive/node-haste#33

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 [email protected]@3 would automatically de-dupe. However, when facebook#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 facebook#5813 Reviewed By: svcscm Differential Revision: D2918380 Pulled By: androidtrunkagent fb-gh-sync-id: 6e4234a45284be2fdf6fedf29e70b2d2d0262486 shipit-source-id: 6e4234a45284be2fdf6fedf29e70b2d2d0262486

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 [email protected]@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 facebook/react-native#5813 Reviewed By: svcscm Differential Revision: D2918380 Pulled By: androidtrunkagent fb-gh-sync-id: 6e4234a45284be2fdf6fedf29e70b2d2d0262486 shipit-source-id: 6e4234a45284be2fdf6fedf29e70b2d2d0262486

Move react to peerDependencies.

376a48f

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Feb 8, 2016

ide reviewed Feb 8, 2016
View reviewed changes

skevy mentioned this pull request Feb 8, 2016

Add npm-shrinkwrap.json to npm #5812

Closed

skevy force-pushed the react-peer-dep branch from 659bf0e to c015d2a Compare February 8, 2016 06:50

skevy force-pushed the react-peer-dep branch from c015d2a to 0ce957e Compare February 8, 2016 06:51

mkonicek reviewed Feb 8, 2016
View reviewed changes

skevy added 2 commits February 8, 2016 12:31

Update local-cli to install react in order to satisfy peerDependency.

9343180

Update 'react-native upgrade' to warn when 'react' is not found in a …

0956e8c

…project's dependencies when the react-native version is >= 0.21.0.

skevy force-pushed the react-peer-dep branch from 0ce957e to 0956e8c Compare February 8, 2016 17:31

frantic assigned martinbigio Feb 8, 2016

frantic mentioned this pull request Feb 9, 2016

Move "react" to be a peer dependency #5541

Closed

ide added GH Review: needs-revision and removed GH Review: review-needed labels Feb 9, 2016

skevy added GH Review: accepted and removed GH Review: needs-revision labels Feb 9, 2016

ghost closed this in 9f01f96 Feb 9, 2016

This was referenced Feb 19, 2016

Merge PRs that get master back to good state #6037

Closed

[0.21] Branch cut #6036

Closed

This pull request was closed.

Move react to peerDependencies #5813

Move react to peerDependencies #5813

Conversation

skevy commented Feb 8, 2016

facebook-github-bot commented Feb 8, 2016

skevy commented Feb 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 8, 2016

facebook-github-bot commented Feb 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkonicek commented Feb 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkonicek commented Feb 8, 2016

skevy commented Feb 8, 2016

facebook-github-bot commented Feb 8, 2016

mkonicek commented Feb 9, 2016

ide commented Feb 9, 2016

ide commented Feb 9, 2016

skevy commented Feb 9, 2016

ide commented Feb 9, 2016

ide commented Feb 9, 2016

facebook-github-bot commented Feb 9, 2016

facebook-github-bot commented Feb 9, 2016

skevy commented Feb 9, 2016

skevy commented Feb 9, 2016

skevy commented Feb 9, 2016

ide commented Feb 9, 2016

skevy commented Feb 9, 2016

mkonicek commented Feb 10, 2016

mkonicek commented Feb 10, 2016

skevy commented Feb 10, 2016

ide commented Feb 10, 2016

davidaurelio commented Feb 15, 2016

mkonicek commented Mar 3, 2016

mkonicek commented Mar 3, 2016

Move `react` to peerDependencies #5813

Move `react` to peerDependencies #5813