quaertym / ember-cli-dependency-checker

Ember CLI addon for checking missing node and bower dependencies before running ember commands
MIT License
40 stars 38 forks source link

Add support for npm-shrinkwrap.json #26

Closed mixonic closed 9 years ago

mixonic commented 9 years ago

The dependency checker is slightly naive, in that it presumes dependencies of dependencies do not require confirmation. We're only talking NPM here.

In practice, we've often run into cases where dependency X has a variable version specified, for example:

{
  name: "torii",
  dependencies: {
    "popup-service": "^1.0.0"
  }
}

When popup-service releases 1.0.1, it is easy to end up with some npm installs having 1.0.0 on disk and others having 1.0.1. The checker has no way to confirm what version should be installed, as that information is not stored in package.json.

Even worse is when 1.0.1 has a bug, but there is no way to force 1.0.0 without forking torii.

The npm shrinkwrap command outputs a summary of what dependencies are currently in node_modules/. There is no way to confirm that what you have node_modules/ matches npm-shrinkwrap.json, but removing the node_modules/ directory and running npm install will result in the versions of npm-shrinkwrap.json being used.

These patches add a new dependency checker to the bower.json and package.json checkers, one that activates when npm-shrinkwrap.json is present and uses it to confirm the contents of the node_modules/ directory is correct.

The ideal way to work with this change is to run the npm shrinkwrap command and commit the resulting npm-shinkwrap.json command after every npm install of a dependency. When other developers (or a build box) pull down the changes the ember command will error if their nested dependencies are incorrect. The suggested resolution to failures is to rm -rf node_modules/ and run npm install. With a blank node_modules/ directory, npm will respect the pinned versions in npm-shrinkwrap.json.

See also the revised README.md.

mixonic commented 9 years ago

Testing with some Real World usage right now, getting a list for cleanup...

mixonic commented 9 years ago

:rocket: :-D

quaertym commented 9 years ago

@mixonic :+1: Thanks, that's a lot of work. I will review and merge asap.

bantic commented 9 years ago

@quaertym Would you also be open to another PR after this that makes creating fixture directory/file-structures on a per-test basis possible? We started pushing the limits of the fixtures when we put this together.

mixonic commented 9 years ago

We have a followup for this after https://github.com/ember-cli/ember-cli/pull/3771 lands, which you review as a diff at https://github.com/201-created/ember-cli-dependency-checker/compare/add-shrinkwrap...201-created:using-node-modules-path. This patch will add support for arbitrary node_modules/ locations to the dependency checker.

But we would like to land this first.

rwjblue commented 9 years ago

LGTM

mixonic commented 9 years ago

@quaertym I'd like to get this and the followup to it merged this week while we have it fresh in our heads, please let me know if we need to make any changes!

quaertym commented 9 years ago

@mixonic I left a few comments let's figure them out, other than those I am fine with the changes. I think this is a great addition to the dependency checker, thanks for the great work.

@bantic I'd like any PR that improves the current situation.

mixonic commented 9 years ago

@quaertym updated for one comment and addressed two others.

quaertym commented 9 years ago

@mixonic Thanks :+1:

mixonic commented 9 years ago

:fireworks: