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

Add npm-shrinkwrap.json to npm #5812

Closed
wants to merge 1 commit into from

Conversation

skevy
Copy link
Contributor

@skevy skevy commented Feb 8, 2016

This probably should have been done a long time ago, but alas, here we are.

This adds the shrinkwrap file to be included when react-native is npm installed.

Previously this wasn't possible due to how we were resolving Babel plugins in the transformer in the packager, but now that we've simplified that and added the preset, this should work fine.

This will be even better when we're able to add react as a peer dependency, rather than a normal dependency.

NOTE: DO NOT MERGE until react is a peer dependency. It will break things for people who use things like Relay/Redux/etc. that depend on React. (#5813 addresses this)

@skevy
Copy link
Contributor Author

skevy commented Feb 8, 2016

cc @vjeux @ide

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @AaaChiuuu, @cpojer and @sahrens to be potential reviewers.

@facebook-github-bot 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
Copy link
Contributor

ide commented Feb 8, 2016

👍

@mkonicek
Copy link
Contributor

mkonicek commented Feb 8, 2016

Sounds awesome to me!

@martinbigio @davidaurelio does this look fine to you?

ghost pushed a commit that referenced this pull request Feb 9, 2016
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 #5813

Reviewed By: svcscm

Differential Revision: D2918380

Pulled By: androidtrunkagent

fb-gh-sync-id: 6e4234a45284be2fdf6fedf29e70b2d2d0262486
shipit-source-id: 6e4234a45284be2fdf6fedf29e70b2d2d0262486
@martinbigio
Copy link
Contributor

Yeah, this is a great change. Would help reduce the number of weird bugs, if any, open-source users run due to using different versions of dependencies. Thanks @skevy!

@martinbigio
Copy link
Contributor

@facebook-github-bot shipit

@facebook-github-bot
Copy link
Contributor

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

@martinbigio
Copy link
Contributor

Please make sure to include this change on the release notes :)

@skevy
Copy link
Contributor Author

skevy commented Feb 13, 2016

Trying to ship again...

@skevy
Copy link
Contributor Author

skevy commented Feb 13, 2016

@facebook-github-bot shipit

@facebook-github-bot
Copy link
Contributor

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

@skevy
Copy link
Contributor Author

skevy commented Feb 14, 2016

@mkonicek Another one of mine that didn't close the PR after the merge...just FYI

@skevy
Copy link
Contributor Author

skevy commented Feb 14, 2016

Closed by: 7586951

@skevy skevy closed this Feb 14, 2016
pglotov pushed a commit to pglotov/react-native that referenced this pull request Mar 15, 2016
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants