Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support monorepos #1677

Merged
merged 4 commits into from Jun 29, 2020
Merged

support monorepos #1677

merged 4 commits into from Jun 29, 2020

Conversation

@micellius
Copy link
Contributor

@micellius micellius commented Apr 29, 2019

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
Copy link

@jsf-clabot jsf-clabot commented Apr 29, 2019

CLA assistant check
All committers have signed the CLA.

micellius added 2 commits Apr 29, 2019
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
Copy link

@redonkulus redonkulus commented May 24, 2019

@vladikoff any way this can get reviewed or merged?

@vladikoff
Copy link
Member

@vladikoff vladikoff commented May 24, 2019

ping @shama 👋

@vladikoff vladikoff requested review from shama and vladikoff May 24, 2019
@redonkulus
Copy link

@redonkulus redonkulus commented May 24, 2019

@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
Copy link
Member

@shama shama commented May 24, 2019

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
Copy link

@redonkulus redonkulus commented May 24, 2019

@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
Copy link
Contributor Author

@micellius micellius commented May 26, 2019

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

Copy link

@redonkulus redonkulus left a comment

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

// 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.

Copy link
Contributor Author

@micellius micellius Jul 16, 2019

@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
Copy link

@Lucaszw Lucaszw commented Jun 24, 2019

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
Copy link

@redonkulus redonkulus commented Jun 24, 2019

@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
Copy link

@Lucaszw Lucaszw commented Jun 27, 2019

@redonkulus Awesome!

@redonkulus
Copy link

@redonkulus redonkulus commented Jul 15, 2019

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

@vladikoff vladikoff merged commit 58f80ae into gruntjs:master Jun 29, 2020
2 checks passed
@redonkulus
Copy link

@redonkulus redonkulus commented Jun 29, 2020

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

@vladikoff
Copy link
Member

@vladikoff vladikoff commented Jun 30, 2020

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

@redonkulus
Copy link

@redonkulus redonkulus commented Jun 30, 2020

@vladikoff that would be perfect, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants