support monorepos by micellius · Pull Request #1677 · gruntjs/grunt

micellius · 2019-04-29T14:56:15Z

allow usage of grunt plugins that are located in any location that
is visible to Node.js and NPM, instead of node_modules directly
inside package that have a dev dependency to these plugins.

Fix #1676.

allow usage of grunt plugins that are located in any location that is visible to Node.js and NPM, instead of `node_modules` directly inside package that have a dev dependency to these plugins.

jsf-clabot · 2019-04-29T14:56:21Z

All committers have signed the CLA.

In `loadNpmTasks()`` try to resolve package location. In case package not found, try to resolve it's location using `require.resolve()`

add integration test for monorepo case, where grunt plugin hoisted to the `node_modules` above package having dev dependency to it

redonkulus · 2019-05-24T19:01:08Z

@vladikoff any way this can get reviewed or merged?

vladikoff · 2019-05-24T19:05:39Z

ping @shama 👋

redonkulus · 2019-05-24T19:17:17Z

@micellius so one use case this may break is if your tasks are not at the root of the package. So currently, I can load a task like this:

// all tasks are located under the '/dist/tasks' folder.
grunt.loadNpmTasks('grunt-foo/dist');

Because loadNpmTasks will take whatever I enter as the path to the module. After your change (I tested this locally), it will fail to load the package because it thinks the package.json is under the dist/ folder. Removing the /dist from loadNpmTasks method fixes that issue, but then Grunt can no longer find the tasks since they are under the dist folder.

Perhaps this use case is unique to me, but I don't think its that farfetched that users might have a src folder that compiles to another folder which could be loaded by Grunt.

Interested to hear your thoughts...

shama · 2019-05-24T19:59:47Z

I think we've intended to support a require() based approach for tasks (search issues) which would handle this use case but the change has never been fully vetted on how it would impact the ecosystem. This change seems to specifically target monorepo architectures though in a more incremental way. I'll play around with it and see if I find any issues or worries we'll need to address first. It might be a good step towards a require() based approach though.

redonkulus · 2019-05-24T21:26:59Z

@shama this would be great. Please keep us posted. In our monorepo, we basically have to copy all the grunt* packages into each packages node_module folder so that Grunt can load everything correctly. It's really hacky and leads to weird states when lerna/npm modify the packages' node_module folders.

In case `loadNpmTasks()` is called with relative path instead of just module name, try to resolve module name and find relevant package.json, but use tasks from specified relative path.

micellius · 2019-05-26T06:57:40Z

Interested to hear your thoughts...

@redonkulus , I pushed the change that takes your use case into account by trying to normalize the argument of loadNpmTasks().

task.loadNpmTasks = function(name) {
  loadTasksMessage('"' + name + '" local Npm module');
  var root = path.resolve('node_modules');
  var pkgpath = path.join(root, name);
  var pkgfile = path.join(pkgpath, 'package.json');
  // If package does not exist where grunt expects it to be,
  // try to find it using Node's package path resolution mechanism
  if (!grunt.file.exists(pkgpath)) {
    var nameParts = name.split('/');
    // In case name points to directory inside module,
    // get real name of the module with respect to scope (if any)
    var normailzedName = (name[0] === '@' ? nameParts.slice(0,2).join('/') : nameParts[0]);
    try {
      pkgfile = require.resolve(normailzedName + '/package.json');
      root = pkgfile.substr(0, pkgfile.length - normailzedName.length - '/package.json'.length);
    } catch (err) {
      grunt.log.error('Local Npm module "' + normailzedName + '" not found. Is it installed?');
      return;
    }
  }
  ...
  // Process task plugins.
  var tasksdir = path.join(root, name, 'tasks');
  if (grunt.file.exists(tasksdir)) {
    loadTasks(tasksdir);
  } else {
    grunt.log.error('Local Npm module "' + name + '" not found. Is it installed?');
  }
}

Now you should be able to use grunt.loadNpmTasks('grunt-foo/dist'); and grunt will understand that this is a path inside a module and take only relevant part of an argument as a module name, while looking for package.json. Logic of tasks directory resolution remains the same and always treats name as path relative to the root

redonkulus

Thanks for making the change. Just one comment on path separators.

redonkulus · 2019-05-26T12:59:23Z

lib/grunt/task.js

+  // If package does not exist where grunt expects it to be,
+  // try to find it using Node's package path resolution mechanism
+  if (!grunt.file.exists(pkgpath)) {
+    var nameParts = name.split('/');


Do you want to use path.sep to support windows?

Looks like there are three more uses of / below too.

@redonculus , I tested my PR on windows and it works fine. There is a need to use path.sep only if someone is using syntax like require(".\\my\\foo\\index.js"). While this may work on windows, it fails on other platform. In opposite, using require("./my/foo/index.js") works on all platforms (including windows). So, practically, I don't see any reason someone would use windows specific paths, if there is a way to write cross-platform code with less typing.

Ok that’s fine then. If it works on windows then I’m good.

Lucaszw · 2019-06-24T14:59:32Z

What's the status on this PR? Any reason it hasn't been merged asides from the path.sep comment mentioned above?

@micellius @redonkulus

redonkulus · 2019-06-24T16:04:08Z

@Lucaszw I have been busy traveling that last few weeks so I haven’t had time to test this yet but will try out the latest changes in this PR this week.

@micellius please see my comment above about using path.sep.

Lucaszw · 2019-06-27T21:19:14Z

@redonkulus Awesome!

redonkulus · 2019-07-15T19:20:13Z

@micellius @Lucaszw I've tested this locally and it works well. Its up to @shama in how he wants to proceed with this PR.

redonkulus · 2020-06-29T23:30:04Z

@vladikoff thanks for merging this in. When do you think this will be released to npm?

vladikoff · 2020-06-30T01:45:08Z

@redonkulus I am thinking to tag this with v1.2.0 this week. How's that?

redonkulus · 2020-06-30T02:47:37Z

@vladikoff that would be perfect, thanks!

support monorepos

d06ca51

allow usage of grunt plugins that are located in any location that is visible to Node.js and NPM, instead of `node_modules` directly inside package that have a dev dependency to these plugins.

micellius added 2 commits April 29, 2019 23:38

support monorepo (as fallback)

f2b118a

In `loadNpmTasks()`` try to resolve package location. In case package not found, try to resolve it's location using `require.resolve()`

support monorepo (test case)

66fc8fa

add integration test for monorepo case, where grunt plugin hoisted to the `node_modules` above package having dev dependency to it

vladikoff requested review from shama and vladikoff May 24, 2019 19:05

support monorepo (relative path)

5a0a02b

In case `loadNpmTasks()` is called with relative path instead of just module name, try to resolve module name and find relevant package.json, but use tasks from specified relative path.

redonkulus reviewed May 26, 2019

View reviewed changes

vladikoff merged commit 58f80ae into gruntjs:master Jun 29, 2020

dependabot bot mentioned this pull request Mar 8, 2021

Bump grunt from 1.0.4 to 1.3.0 in /webapps langfr/camunda-bpm-platform#77

Closed

larowlan mentioned this pull request May 8, 2021

Bump grunt from 1.0.4 to 1.3.0 larowlan/drupalbris-twig-jest#7

Closed

This was referenced Jul 4, 2021

Update dependency grunt to v1 [SECURITY] byteshiva/WebUtilities#4

Merged

Update dependency grunt to v1 [SECURITY] byteshiva/byteshiva_temp#5

Merged

renovate bot mentioned this pull request Jul 5, 2021

Update dependency grunt to v1 rxcod9/joy-backpack-api-laravel-demo#18

Open

1 task

mend-for-github-com bot mentioned this pull request Jul 14, 2021

Update dependency grunt to v1.3.0 joshnewton31080/NodeGoat_JOSH#221

Open

1 task

renovate bot mentioned this pull request Jul 18, 2021

Update dependency grunt to v1.3.0 [SECURITY] - autoclosed Jakub41/Experimentarium#4

Closed

1 task

This was referenced Aug 17, 2021

Update dependency grunt to v1.3.0 - autoclosed Dima2021/NodeGoat#198

Closed

Pin dependencies Dima2021/NodeGoat#205

Open

Conversation

micellius commented Apr 29, 2019

Uh oh!

jsf-clabot commented Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

redonkulus commented May 24, 2019

Uh oh!

vladikoff commented May 24, 2019

Uh oh!

redonkulus commented May 24, 2019

Uh oh!

shama commented May 24, 2019

Uh oh!

redonkulus commented May 24, 2019

Uh oh!

micellius commented May 26, 2019

Uh oh!

redonkulus left a comment

Choose a reason for hiding this comment

Uh oh!

redonkulus May 26, 2019

Choose a reason for hiding this comment

Uh oh!

micellius Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

redonkulus Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

Lucaszw commented Jun 24, 2019

Uh oh!

redonkulus commented Jun 24, 2019

Uh oh!

Lucaszw commented Jun 27, 2019

Uh oh!

redonkulus commented Jul 15, 2019

Uh oh!

redonkulus commented Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vladikoff commented Jun 30, 2020

Uh oh!

redonkulus commented Jun 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jsf-clabot commented Apr 29, 2019 •

edited

Loading

redonkulus commented Jun 29, 2020 •

edited

Loading