grunt icon indicating copy to clipboard operation
grunt copied to clipboard

npm@3 and peerDependencies

Open iarna opened this issue 10 years ago • 8 comments

You may already be aware, but npm@3 introduces breaking changes to how peerDependencies are handled. Specifically, it no longer installs them for users. INSTEAD, it reports warnings if they're missing or invalid, but otherwise leaves it to the end user to solve.

I gather that grunt plugins regularly peerDep on grunt itself, which makes sense. This is the use case that peerDependencies were originally designed for. Where your users may run into issues is if they were using peerDependencies to do cross-plugin dependencies. In these cases I would recommend making them ordinary dependencies.

In checking a random handful of grunt plugins I didn't see any that ran afoul of the problem above, but still, I wanted to make sure you all were aware of the change.

iarna avatar Jul 06 '15 20:07 iarna

Thanks for the heads up @iarna (and for all the great work on npm), we're aware!

tkellen avatar Jul 06 '15 20:07 tkellen

coffee-script -> peerDependencies It seems reasonable....

monolithed avatar Aug 08 '15 20:08 monolithed

I ran into this issue today when I used npm@3 to install grunt-not-constantinople, which has peerDependencies on grunt-contrib-clean, grunt-istanbul, and grunt-istanbul-coverage.

Realistically, the problem is in how grunt.loadNpmTasks/grunt.task.loadNpmTasks works... it is very specifically written to work in the npm@<3 world as it relies on a node_modules directory existing as a child of the main directory rather than using a more robust mechanism like require.resolve to traverse into module directories.

Ironically, with the current version of Grunt and npm@3, the grunt-loadNpmTasks Node module that I wrote a long time ago for a hierarchical codebase would now actually be applicable to a non-hierarchical codebase to solve this issue, too. :confused:

JamesMGreene avatar Jan 27 '16 18:01 JamesMGreene

I'm hesitant to change loadNpmTasks() atm and I don't think require.resolve() will fully fix that problem.

At present, loadNpmTasks(name) will look in path.join(process.cwd(), 'node_modules', name, 'tasks') and load every file from within. require.resolve will resolve a single file based on a set of node paths and the "main" field in the package.json.

If a plugin splits their tasks into multiple files, loading via require.resolve won't work. Also many plugin unfortunately still have "main": "Gruntfile.js" in their package.json due to a default in one of our init scripts a long time ago. Things could go pretty wrong accidentally loading another project's Gruntfile.

Plus any changes to those in general might cause backwards compatibility issues. Which means a fractured plugin ecosystem as we release new Grunt versions.


One idea is to expose the loadTask() method. Then plugin authors could explicitly require and load any additional plugins required by their task:

grunt.loadTask(require.resolve('grunt-plugin/tasks/plugin.js'));

My only concern with this is the task registry is currently a global thing. Loading a npm task that in turn loads additional tasks into that register will clobber each other and the end user would be unaware.


Another idea is to have registerTask and registerMultiTask return a function. In the future, I'd like if all Grunt tasks could be exported and used standalone:

var grunt = require('grunt');
module.exports = grunt.registerTask('my-task', function(foo) {
  grunt.log.writeln(foo + ' works');
});
// ... in another file or plugin ...
var myTask = require('my-plugin');
myTask('it');

This way if a plugin depended on another plugin, one would just install as a dependency and use that dependency. The tasks they use would remain encapsulated and no peer dependencies required. With the added benefits that testing would become easier and Grunt tasks play nice with the rest of the npm ecosystem.

I have plans to create a rfc for this and further explore it but want to release v1.0.0 before working on any rfcs.

shama avatar Jan 29 '16 22:01 shama

@shama: All I'm really saying is to either utilize the require.resolve machinery to find the appropriate base directory (in a slightly roundabout fashion in some cases) or else do a findup for "node_modules/<%= taskName %>/tasks" (rather than assuming they will be in the plugin's "node_modules" sub-directory as in npm@<3 structure).

Seems easy and reasonable without any realistic breakage.

JamesMGreene avatar Jan 30 '16 22:01 JamesMGreene

https://github.com/gruntjs/grunt-next/blob/master/runner/index.js

tkellen avatar Jan 30 '16 22:01 tkellen

@tkellen:

JamesMGreene avatar Jan 30 '16 22:01 JamesMGreene

@JamesMGreene Modifying that API runs the risk of making some plugins only compatible with [email protected] while others only [email protected]. If we are going to modify that API I'd prefer to fix the issue all together and remove Grunt (or any other plugin) as a peer dependency requirement.

shama avatar Jan 30 '16 22:01 shama