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

[npm] Add the semver caret to package.json deps #1736

Closed
wants to merge 1 commit into from

Conversation

ide
Copy link
Contributor

@ide ide commented Jun 24, 2015

The caret is standard for npm --save and makes it easier to get bugfixes from package maintainers.

As I understand it, Facebook pins the versions since the packages are manually downloaded. One issue with this is that the dependencies of dependencies aren't pinned so what probably makes the most sense for Facebook is to use npm shrinkwrap which recursively pins the versions of all dependencies. How this works is:

  1. Someone at Facebook manually downloads the latest version of an npm package they want to use
  2. They add the entry to package.json using the caret, ex: "babel": "^5.6.5"
  3. They run npm shrinkwrap to generate npm-shrinkwrap.json so the exact versions of the packages are recorded and if you were to run npm install then it would know to get those specific versions (example npm-shrinkwrap.json: https://gist.github.com/ide/a2f6e6818802e39978d5)
  4. package.json is synced out to GitHub but npm-shrinkwrap.json is not
  5. External users run npm install and get the latest compatible versions of all packages

Test Plan: Ran UIExplorer and tested the Chrome debugger.

@ide ide mentioned this pull request Jun 24, 2015
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 24, 2015
The caret is standard for `npm --save` and makes it easier to get bugfixes from package maintainers.

As I understand it, Facebook pins the versions since the packages are manually downloaded. One issue with this is that the dependencies of dependencies aren't pinned so what probably makes the most sense for Facebook is to use `npm shrinkwrap` which recursively pins the versions of all dependencies. How this works is:

1. Someone at Facebook manually downloads the latest version of an npm package they want to use
2. They add the entry to package.json using the caret, ex: `"babel": "^5.6.5"`
3. They run `npm shrinkwrap` to generate npm-shrinkwrap.json so the exact versions of the packages are recorded and if you were to run `npm install` then it would know to get those specific versions (example npm-shrinkwrap.json: https://gist.github.com/ide/a2f6e6818802e39978d5)
4. package.json is synced out to GitHub but npm-shrinkwrap.json is not
5. External users run `npm install` and get the latest compatible versions of all packages

Test Plan: Ran UIExplorer and tested the Chrome debugger.
@frantic
Copy link
Contributor

frantic commented Jun 26, 2015

package.json is synced out to GitHub but npm-shrinkwrap.json is not

Why? We are still going to check in all npm modules internally, but @amasad and I were thinking about using shrinkwrap for opensrouce version to make sure we are using the same modules and don't have weird inconsistencies.

@ide
Copy link
Contributor Author

ide commented Jun 26, 2015

Yes, you could ship with npm-shrinkwrap.json since users always have the option to install with --no-shrinkwrap.

Personally I would still try publishing react-native without npm-shrinkwrap.json because it offers users more flexibility with dependency versions so that "npm dedupe" is more likely to be able to remove duplicate packages, and it lets users automatically receive bugfixes from package authors. The majority of npm packages aren't shrinkwrapped and it's been working well. If it ends up being a pain then I think publishing the shrinkwrap JSON is ok.

@ide
Copy link
Contributor Author

ide commented Jun 26, 2015

Regarding package deduping, npm 3 (announced just a couple hours ago!) will dedupe by default: https://github.com/npm/npm/releases/tag/v3.0.0. As a realistic example, if an app depends on the "promise" package this means npm will try to set up this folder hierarchy:

node_modules
  promise
  react-native
    node_modules
      ... everything except promise

You guys probably have done some perf experiments showing how important it is to minimize JS evaluation on mobile so this optimization is pretty exciting.

Continuing with the example, react-native is currently using promise 7.0.3. In the future, say that I want to use a new feature or optimization in promise 7.1.0 when it comes out. Assuming the maintainer of the promise package is following semver (most people try to be good about it), this should be a backwards-compatible upgrade.

If react-native is pinned to promise 7.0.3 exactly, then npm will set up this:

node_modules
  promise@7.1.0
  react-native
    node_modules
      promise@7.0.3
      ... everything else

and now the app loses the benefits of deduping.

So the tl;dr is: the carets in package.json let me get this deduping optimization. If you guys decide to ship with npm-shrinkwrap.json I can still get the optimization by running npm install --no-shrinkwrap. If you leave out npm-shrinkwrap.json then everyone will get the optimization by default (but at the risk of a package maintainer not following semver).

@sahrens
Copy link
Contributor

sahrens commented Jul 15, 2015

ping on this...having our internally checked-in packages slowly grow out of date with OSS is not a good situation. Perhaps we could add a lint rule or unit test to verify the versions checked into node_modules internally match the versions in package.json?

Also: #1874

@sahrens
Copy link
Contributor

sahrens commented Jul 15, 2015

@frantic is there a reason you accepted this and didn't import it?

@frantic
Copy link
Contributor

frantic commented Jul 16, 2015

I need to chat with @amasad about this a bit. Definitely see @ide's point and benefits of using ^, however this indeed reveals internal updates problem. We'll have to be constantly updating versions of internally checked-in modules to match opensource.

ide referenced this pull request in sahrens/react-native Sep 5, 2015
@vjeux
Copy link
Contributor

vjeux commented Sep 5, 2015

@ide if we were to add ^, do npm dedupe ourself and provide a shrinkwrap. Would we get the benefits of dedupe for everyone AND not be impacted by new versions breaking things? If that's the case, seems like the best of both worlds and we should do that.

@ide
Copy link
Contributor Author

ide commented Sep 5, 2015

@vjeux that would be better since it would dedupe react-native's own recursive dependencies. But if you ship npm-shrinkwrap.json then I think the carets are ignored. The reason I want the carets is so that I can install react-native with --no-shrinkwrap and get the latest bugfixes and maximum opportunity for deduping.

I would have to test with npm 3 to see if it dedupes across react-native and other packages in the app's node_modules. For example, say react-native's npm-shrinkwrap.json asks for pkg 1.2.0 and another one of my node_modules depends on pkg ^1.0.0. If pkg is up to 1.3.0, I am not sure if npm

  • optimizes for deduping and downloads only pkg 1.2.0 because that satisfies both requirements
  • optimizes for getting the latest bugfixes and downloads pkg 1.3.0 for my other node_module and downloads 1.2.0 for react-native because npm-shrinkwrap.json asks for it

@vjeux
Copy link
Contributor

vjeux commented Sep 5, 2015

get the latest bugfixes and maximum opportunity for deduping.

You may want that as a power user but we don't want that for react-native, the project. We've had several breakages already because of npm version mismatch internally and externally which doesn't follow semver.

The project covers a huge surface area and is not super robust at the moment, I really want to be able to lock down this avenue for breakages that we have no control over and is super hard to troubleshot.

So, if we lock down by default but you can do --no-shrinkwrap, I think that we achieve the best possible outcome.

@ide
Copy link
Contributor Author

ide commented Sep 5, 2015

Here are the results with npm 3, which appears to optimize for getting the latest packages and applies deduping afterwards.

I created two packages, pkg-not-shrinkwrapped and pkg-shrinkwrapped that both depend on "lodash": "^3.0.0":

{
  "name": "pkg-shrinkwrapped", // and pkg-not-shrinkwrapped
  "version": "1.0.0",
  "main": "index.js",
  "dependencies": {
    "lodash": "^3.0.0"
  }
}

In pkg-shrinkwrapped I created an npm-shrinkwrap.json file that pinned lodash to 3.0.0 exactly.

So the question was: if you install both pkg-shrinkwrapped and pkg-not-shrinkwrapped with npm 3 does it

  • share lodash 3.0.0 across both packages... or
  • does it get lodash 3.10.1 for pkg-not-shrinkwrapped and 3.0.0 for pkg-shrinkwrapped and try to dedupe later?

It turns out that npm optimizes for getting the latest packages and fetches lodash 3.10.1 for pkg-not-shrinkwrapped:

.
├── node_modules
│   ├── lodash  // 3.10.1
│   ├── pkg-not-shrinkwrapped
│   │   └── package.json
│   └── pkg-shrinkwrapped
│       ├── node_modules
│       │   └── lodash  // 3.0.0
│       ├── npm-shrinkwrap.json
│       └── package.json
└── package.json

Going back to react-native, this means that if react-native ships with npm-shrinkwrap.json that asks for lodash 3.0.0 then it won't be deduped with other copies of lodash in my app unless my other node_modules ask for lodash 3.0.0 exactly instead of ^3.0.0.

The cool thing is if I trust semver, I can just run npm i --no-shrinkwrap and npm 3 will fetch only lodash 3.10.1 and share it between pkg-not-shrinkwrapped and pkg-shrinkwrapped in my test sample. This is what I want to do with react-native and why I want package.json to specify carets.

This way most users who run npm install will follow npm-shrinkwrap.json and ignore the carets, and people like me can ignore npm-shrinkwrap.json and get the latest packages and more deduping.

@ide
Copy link
Contributor Author

ide commented Sep 5, 2015

So, if we lock down by default but you can do --no-shrinkwrap, I think that we achieve the best possible outcome.

Yes. This sounds great. I just want the carets in package.json so that I get more flexible dependency ranges when I ignore npm-shrinkwrap.json. I believe we're on the same page.

@vjeux
Copy link
Contributor

vjeux commented Sep 5, 2015

Cool let's do that!

@frantic
Copy link
Contributor

frantic commented Sep 30, 2015

@tadeuzagallo shrinkwrapped all the things, closing this one

@frantic frantic closed this Sep 30, 2015
@ide
Copy link
Contributor Author

ide commented Sep 30, 2015

champagne

ghost pushed a commit that referenced this pull request Oct 1, 2015
Summary: @​cesarandreu pointed out running eslint on react-native resulted in `t.isReferencedIdentifier is not a function` issues on the babel #linting channel on slack.

- update eslint, babel-eslint, eslint-plugin-react
- fix eslint errors: `Error - t.isReferencedIdentifier is not a function`
- fix lint npm script

I see there's also #1736 from @ide which would fix it tooCloses #1874

Reviewed By: @vjeux

Differential Revision: D2495878

Pulled By: @frantic
MattFoley pushed a commit to skillz/react-native that referenced this pull request Nov 9, 2015
Summary: @​cesarandreu pointed out running eslint on react-native resulted in `t.isReferencedIdentifier is not a function` issues on the babel #linting channel on slack.

- update eslint, babel-eslint, eslint-plugin-react
- fix eslint errors: `Error - t.isReferencedIdentifier is not a function`
- fix lint npm script

I see there's also facebook#1736 from @ide which would fix it tooCloses facebook#1874

Reviewed By: @vjeux

Differential Revision: D2495878

Pulled By: @frantic
Crash-- pushed a commit to Crash--/react-native that referenced this pull request Dec 24, 2015
Summary: @​cesarandreu pointed out running eslint on react-native resulted in `t.isReferencedIdentifier is not a function` issues on the babel #linting channel on slack.

- update eslint, babel-eslint, eslint-plugin-react
- fix eslint errors: `Error - t.isReferencedIdentifier is not a function`
- fix lint npm script

I see there's also facebook#1736 from @ide which would fix it tooCloses facebook#1874

Reviewed By: @vjeux

Differential Revision: D2495878

Pulled By: @frantic
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