fix(workspace): delegate to local ng script to allow customization

[juristr]

Current Behavior (This is the behavior we have today, before the PR is merged)

When invoking the various NX CLI commands in non-parallel mode, it directly invokes the ng binary in the node_modules folder. That prevents applying customizations such as setting the memory limits.

Expected Behavior (This is the new behavior we can expect after the PR is merged)

The behavior should be unified between parallel execution (which uses the ng script in the package.json) and the non-parallel execution.

Issue

#1110

[juristr]

@vsavkin Just a quick question. I pushed this WIP PR with a quick & dirty implementation of changing the current implementation, s.t. it uses the ng script in the package.json. Still need to do some further testing and some refactoring.

I was just wondering, because in the implementation right now I've used the npm-run-all script but passing in false for the parallel option since we want to execute it serially. The code could actually now be refactored between the parallel & serial execution as it differs only in a couple of flags. However I noticed that for the parallel execution, the failedProjects don't seem to be used:

nx/packages/workspace/src/command-line/affected.ts

Line 232 in e88baba

let failedProjects = [];

Is this intentional and should be preserved?

[alfaproject]

I'm actually surprised there's a need to rely on the user to have the ng script in package.json. Why not always use the package from node modules directly in all cases?

[juristr]

@alfaproject well that would be another option. but based on a discussion with @vsavkin relying on the package.json script allows the user to better customize the invocation of the ng script. Otherwise it would be hidden by NX

[alfaproject]

Right, I guess there's a valid argument for both ways

[vsavkin]

@juristr I like your implementation. I think using npm-run-all is a good idea.

I think we should use failedProjects for parallel execution as well. I imagine it was easier to do it the way it is done.

[juristr]

@vsavkin Alright awesome. Gonna clean up the implementation and complete this PR 👍

[juristr]

Alright, should be ready for review @vsavkin. There are some tests that aren't passing (even on master), didn't have the time to check that though.

Another thing I noticed, is that the message "You can isolate the above projects by passing --only-failed" is not always printed. Probably due to this line

https://github.com/nrwl/nx/blob/master/packages/workspace/src/command-line/workspace-results.ts#L36

..if the failing project is the 1st one to run, then the message isn't being printed. Although IMHO it should. Do you know what's the reasoning behind startedWithFailedProjects flag? What is it for?

[vsavkin]

@juristr CI passed, so it might have been something on your local.

I think the reasoning behind startedWithFailedProjects is to tell the user that even though the provided command passed, it doesn't mean that the build is green and that they need to rerun it without the flag.

I think the PR looks good. I'm going to merge it. Thank you @juristr !

[juristr]

CI passed, so it might have been something on your local.

There was actually a breaking integration test which I fixed and pushed yesterday in this commit 🙂

[github-actions]

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.