[WIP] Support for the --save and --save-dev flags

[davej]

Just doing an early pr so you can take a look and let me know if I'm on the correct path or if you have other ideas about how it should be implemented.

[rstacruz]

looks good, what's missing from this one?

[rstacruz]

use require('npm-package-arg') here... it figures that out exactly for you.

[rstacruz]

hmm, the npm behavior is to do:

npm install --save rimraf

and it'll pick up rimraf@^2.5.1 for you based on what it resolves. Might simply need to make installMultiple() throw data of the top-level packages it installed.

[rstacruz]

I think a neater way to do this is to make installMultiple() work like this:

installMultiple(...)
  .then(packages => {
    // sift through `packages` for ones that match `cli.input`, and save those
  })

to do this, install() should simply resolve to the necessary package data at the end:

function install (...) {
  // these stuff are already available in `install()`.
  var pkg = {
    name: 'lodash',
    fulldata: /* package.json here */,
    ...
  }

  return /* snip */
    .then(_ => pkg) // <-- resolve the Promise to that stuff so that it can be picked up later
}

[davej]

Yeah, you're correct. Have a better grasp of what I'm trying to do now. I'll pick this up tomorrow, I have to head out the door now.

[davej]

I think all that's missing is support for other install types:

• @Scoped packages (npm i @rstacruz/tap-spec)
• tarball release packages (npm i http://foo.com/tar.tgz)

[rstacruz]

oh, this has merge conflicts... but i rebased your branch for you (pr-41-rebased). You can pick it up like so:

git remote add upstream https://github.com/rstacruz/pnpm.git
git fetch upstream master
git reset --hard upstream/pr-41-rebased

[rstacruz]

by the way, huge thanks! really appreciate the help.

[davej]

Hey @rstacruz, could you take another look at this now. By the way, tests will break if the @rstacruz/tap-spec module is updated to a new version.

[rstacruz]

neat! resting now but ill look at this tomorrow.

btw, how's the code base so far? did you have trouble finding your way through?

[rstacruz]

everything looks ok so far, havent tested it though.

[davej]

@rstacruz, yeah codebase is very good, didn't take me too long to ramp up. Makes sense to limit the scope to npm install too instead of trying to reimplement all of npm. I would even consider dropping --global support to be honest.

Would be good to get some CI performance benchmarks in there to make sure that we're keeping track of performance as the codebase evolves.

[rstacruz]

I would even consider dropping --global support to be honest.

good point, maybe --global can just be a passthru to vanilla npm install. there may be scary voodoo involved in making a pnpm install --global work interoperably with non-pnpm global modules... would rather not think about it if it doesn't really matter.

[davej]

Yeah, unless there's a simple way to do it that is 100% compatible with npm then I'd drop it. Day-to-day I don't do a whole lot of npm install -g so it's performance doesn't stress me out like local install does.

[rstacruz]

instead of trying to reimplement all of npm

I'm also planning to reimplement whatever in npm that isn't compatible with pnpm... at the moment the only other thing (afaik) is npm uninstall.

[rstacruz]

ah hell, merging this now and trusting CI, haha. thanks for your help!

[davej]

Woah, hold up, I just noticed that I overwrote some existing tests with this pr :)

[rstacruz]

Oops!

[davej]

I guess we can manually add them back in. I think what happened is that I was editing the file in my text editor when I updated to your rebased branch.

[rstacruz]

btw, -O/--save-optional also exists, forgot about that

[rstacruz]

or:

require('mz/fs').writeFile

[davej]

Yup. One other thing that I noticed, read-pkg-up does a lot of normalization, I'm not sure that we want all of it?

For example optionalDependencies get added to dependencies: https://github.com/npm/normalize-package-data#what-normalization-currently-entails

[rstacruz]

oh we definitely don't want to do that. using lib/require_json.js might be in order.

[davej]

Do you want me to fix this pr, add the overwritten tests and squash it or will I let you fix it?

[rstacruz]

a new pr would probably be better.

[davej]

ok