New Options (dev, peerDependencies)

[ericclemmons]

Use case: allowing configuration of the npm install options used to vary based on the module missing dependencies were detected in.

This is pretty niche, but would be neat DX touch for tooling which can auto-configure it for you.

e.g. if installation is triggered by a module under src/, use --save; a module under test/, use --save-dev

Potential API (which suits a plugin, as it would otherwise need to be top-levelled in webpack config for a loader): pass a RegExp as save/saveDev config which is tested against the relative path from process.cwd() to the module the current install is being triggered from:

cli: {
  save: /^src\//,
  saveDev: /^test\//
}

Alternatively, allow cli or any of its props to be a Function which would take a module arg similar to the minChunks option for CommonsChunkPlugin. Seems a bit overkilly, but you never know.

[ericclemmons]

Great request! What about this as well?

cli: (somethign) => ({
  const save = ...;
  const saveDev = ...;
  return { save, saveDev };
}),

Maybe that's what you meant by by the last suggestion? (I haven't used CommonsChunkPlugin in a while, but I like the callback approach, personally. Do you have an example of their usage?)

[ericclemmons]

I may have this in #5. This is especially useful with the cache-min option for making npm-install much faster during development.

[ericclemmons]

Even though it's repetitious, I think following the same format as externals works best:

• Argument values
  • Boolean (e.g. true, false)
  • Number (e.g. cacheMin: 999999 correctly gets the number, not the string)
  • String (e.g. registry=...)
• Regex,
  • Boolean result of result.request.match(...)
• Function
  • Value returned from function with signature (result, dependency).

[ericclemmons]

Going to save this for after 2.0.0. It should be pretty easy, but there was enough movement for #5 alone :)

[bebraw]

I would love to see this happen. To quote from #22:

The reason for this is that I prefer to keep strict separation between dependencies and devDependencies. For example all ./app dependencies would go to the former category whereas the rest would go to the latter.

I think this would be possible with two rules. In pseudocode "If in ./app do save else do save-dev".

I would be totally happy with a callback where I get access to the path and can return save or save-dev (or something else). That would be more flexible than a regex, though you can still use regex there if you want.

[bebraw]

I gave it a quick go. Something like this would work for me:

plugin.js

...

NpmInstallPlugin.prototype.resolve = function(request, path) {
  var options = this.options;
  var dep = installer.check(request);
  var resolve = options.resolve || function() {return {};}

  if (dep) {
    installer.install(dep, Object.assign({}, options, resolve(path)));
  }

  return dep;
};

NpmInstallPlugin.prototype.resolveModule = function(result, next) {
  // Only install direct dependencies, not sub-dependencies
  if (!result.path.match("node_modules")) {
    this.resolve(result.request, result.path);
  }

  next();
};

// probably pass path for loaders too?
...

resolve is a callback that receives the path of the match. You can then inject npm options as you like. Even though simple, this interface is quite powerful and enough for my purposes.

Now the paths passed to the callback are full. Maybe it would make sense to resolve them against the project root. I'm not sure about this. This is something that can be done on the user side, though.

[ericclemmons]

I agree, the callback is clearer IMO, since you explicitly get a method
signature.

FYI, there's some major re-archtecting happening for resolve.root and
resolve.alias support, so I'd hold off on any PRs.

Thanks for the working example! That shaved off lots of time for me :)

[bebraw]

I agree, the callback is clearer IMO, since you explicitly get a method signature.

I gave this some extra thought. It feels like returning paths resolved against cwd would be neatest. Then you could match the path against a regex like /^src\// easily (@insin's case). That's probably what you need 90% of the time. Something more exotic is still possible of course.

FYI, there's some major re-archtecting happening for resolve.root and resolve.alias support, so I'd hold off on any PRs.

Ok, cool. Let me know if I can be of any help. I would love to see these features land soon. 👍

This plugin is going to cut tons of npm installs out of the book. 😄

[ericclemmons]

v3.0.0 is out with resolve.root and resolve.alias support, so this would be really useful now!

[bebraw]

@ericclemmons Do you want a PR?

[ericclemmons]

Yes, this is quite useful.

I want to wait for #29 to land, which is top priority for me. That single PR may change a lot of the code behind the scenes, and I hate merge conflicts ;)

[ericclemmons]

This will be the last feature added for v3.1.0.

[ericclemmons]

Ok, I'm going to implement a RegExp for matching context, per @insin's suggestion.

In 4.0, I'm going to remove these options entirely in favor of an alternative.

For reference, see how externals work with function signatures:

https://webpack.github.io/docs/configuration.html#externals

I'm considering making this a breaking change. save and saveDev shouldn't both be true, but at least one should be true.

(npm install without saving causes more confusion than anything, and this project will continue re-installing until it's in package.json).

@insin's example using context is probably the best indicator. Otherwise, you'd have to test for the package name, which is greatly up to how the author leverages it. (I, for example, run webpack & babel on production).

Also, passing the CLI options is actually best handled in .npmrc. I can't see a reason to support arbitrary CLI args.

Instead, we'll probably do something like:

new NpmInstallPlugin({
  // Plugin noops in other environments
  install:  process.env.NODE_ENV === "development",
  // Choose `save` or `save-dev`
  dev: /test/, // defaults to false
});

[ericclemmons]

Ugh, this also means doing the RegExp checking before calling installer.install, since it has the function signature install(dep, options).

[ericclemmons]

I think I'd rather release v3.1.0, and put this into v4.

[ericclemmons]

Thinking about this some more today:

• It's not reasonable to rely on .npmrc for something like --save or --save-dev.
• Saving is a requirement for the plugin to work.
• Preventing this plugin from running in production is a configuration problem.
• The only "switch" to be flipped is really dev: true|false, to indicate something is a devDependencies.
• The regular expression is a smaller pattern, but not clear whether it's a path or package name.

I think the best solution, AFAIK is:

• Default dev to false. Assume people build apps first, tests later. When using babel or webpack, there are no guarantees what is needed for the production environment.

• To start, only support Boolean or Function (that returns a Boolean):

  dev: NODE_ENV === "test",

  or

  dev: function(path, module) {
    return path.match(/test/) ? true : module.match(/eslint/);
  }

I'm sure there's some dilemma when a dependency is saved in both spots, but at some point the user is in charge of their project's package.json.

This project is to help simplify things, not replace them.

[ericclemmons]

Shoot. In the interest of having this added sooner, I may opt for:

dev: function(module) {
  return module.match(/ava|mocha|eslint|babel/);
}

Why? Because currently the installer.js file is completely unaware of the request/context/result, only the module, and it'll be a PITA to pull this out of the errors that show up from missing modules.

Again, I'm interested primarily in preventing the errors that users have with cyclical installs because they're not passing save or saveDev options (or specifying .npmrc) first.

Then adding utility.

[ericclemmons]

Nevermind. I think I got it. Sorry for the spam.

[ericclemmons]

Ok, this was easier than I thought :)

Stupid simple example:

    new NpmInstallPlugin({
      dev: function(module, path) {
        return [
          "babel-preset-react-hmre",
          "webpack-dev-middleware",
          "webpack-hot-middleware",
        ].indexOf(module) !== -1;
      },

      peerDependencies: true,
    }),

Defaults are:

{ dev: false, peerDependencies: true };

Next up:

• Test the remaining examples.
• Fix tests.
• Release v4!

[coveralls]

Coverage decreased (-1.3%) to 93.452% when pulling 0260a2a on 13-dev into b8b8d29 on master.

[coveralls]

Coverage decreased (-1.3%) to 93.452% when pulling c67e0fe on 13-dev into b8b8d29 on master.