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

fix(angular): support maxParallel, parallel, and withDeps options in webpack-browser schema #4548

Closed
wants to merge 3 commits into from
Closed

fix(angular): support maxParallel, parallel, and withDeps options in webpack-browser schema #4548

wants to merge 3 commits into from

Conversation

LayZeeDK
Copy link
Contributor

@LayZeeDK LayZeeDK commented Jan 17, 2021

Current Behavior

The withDeps option is required for incremental build when using the @nrwl/angular:webpack-browser executor as briefly mentioned in Setup incremental builds for Angular applications > Running and serving incremental builds.

This guide also mentions that we can configure withDeps and parallel in the options of the incremental serve target of our workspace configuration. It doesn't mention this for the build target.

However, the maxParallel, parallel, or withDeps options are not declared in the executor schema or its documentation.

Expected Behavior

Seeing as withDeps is required for the incremental build target to work, we should add support for the maxParallel, parallel, and withDeps options in the schema of @nrwl/angular:webpack-browser.

Related Issue(s)

Fixes #

Other information

We're building an Nx plugin to convert an application and its library dependencies to use incremental build and this is blocking us from implementing it for the best end-developer experience.

See nx-worker/nxworker#5.

@nx-cloud
Copy link

nx-cloud bot commented Jan 17, 2021

Nx Cloud Report

CI ran the following commands for commit 126a537. Click to see the status, the terminal output, and the build insights.

Status Command Start Time
#000000 nx build-base angular 1/17/2021, 11:31:33 AM
#000000 nx run-many --target=build --all --parallel 1/17/2021, 11:31:29 AM
#000000 nx run-many --target=e2e --projects=e2e-angular 1/17/2021, 11:36:14 AM
#000000 nx run-many --target=lint --all --parallel 1/17/2021, 11:38:31 AM
#000000 nx run-many --target=test --all --parallel 1/17/2021, 11:34:55 AM

Sent with 💌 from NxCloud.

@LayZeeDK LayZeeDK marked this pull request as ready for review January 17, 2021 13:01
@LayZeeDK
Copy link
Contributor Author

LayZeeDK commented Jan 17, 2021

Looking for guidance on how to pass this test failure in angular-e2e:e2e:

>  NX   ERROR  Running target "build" failed
      
        Failed projects:
        
        - app7978734
      
       - Compiling TypeScript sources through NGC
      ✔ Compiling TypeScript sources through NGC
      - Writing package metadata
      ✔ Writing package metadata
      ℹ Built @proj/buildlib16237231
      Schema validation failed with the following errors:
        Data path "" should NOT have additional properties(withDeps).
      npm ERR! code ELIFECYCLE
      npm ERR! errno 1
      npm ERR! [email protected] nx: `nx "build" "app7978734" "--with-deps"`
      npm ERR! Exit status 1
      npm ERR! 
      npm ERR! Failed at the [email protected] nx script.
      npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
      
      npm ERR! A complete log of this run can be found in:
      npm ERR!     /home/circleci/.npm/_logs/2021-01-17T12_56_34_000Z-debug.log

      at Object.runCLI (../utils/index.ts:308:15)

@vsavkin vsavkin self-assigned this Jan 21, 2021
@vsavkin
Copy link
Member

vsavkin commented Feb 1, 2021

@LayZeeDK sorry for the late reply and thank you for the PR.

maxParallel, parallel, or withDeps are used by the Nx tasks runner, and are not used by the executor. The executor doesn't schedule the tasks to build the dependencies, the tasks runner does. You can set maxParallel and parallel in nx.json in the runner's options. You cannot set withDeps right now.

Is you concern the lack of docs? Or the fact that you have to do: nx build myapp --with-deps?

@LayZeeDK
Copy link
Contributor Author

LayZeeDK commented Feb 1, 2021

Hello @vsavkin,

Thank you for your reply.

I was confused because you yourself added these options to the @nrwl/web:file-server executor in this commit:
7d3627a

Isn't it possible to add these parameters to target options in workspace.json?

@vsavkin
Copy link
Member

vsavkin commented Feb 11, 2021

When nx runs any command it reads the tasks runner config from nx.json. This is how it knows what tasks runner to use, what options it has etc. The workspace.json file lists the tasks and configures the executors/builders.

The file-server target is a special case cause it actually invokes nx run under the hood, so it can pass --parallel and --maxParallel to it. Maybe I should have named those properties buildParallel and buildMaxParallel to indicate that those just happen to match the stuff in nx.json, but they didn't have to match. The file-server executor could have had its own way of running things in parallel which had nothing to do with the tasks runner. It just happened to invoke Nx under the hood.

@LayZeeDK
Copy link
Contributor Author

I see, @vsavkin. Is there any way we can add these options to for example build targets in workspace.json?

@vsavkin
Copy link
Member

vsavkin commented Feb 22, 2021

@LayZeeDK it depends on the build target. You can have a build target that actually invokes Nx under the hood (as the file-server target does), in this case we could add them. The executor implementing the target has to be able to interpret those params. For instance, the jest executor has a param called maxWorkers, which is analogous to maxParallel.

The options in workspace.json configure targets' executors. When you run things like nx build myapp --with-deps --parallel, you actually run many targets, so many executors are invoked with their own configurations. myapp, in this case, isn't built in parallel. The set of [myapp, mylib, mylib2, ..] is built in parallel. The parallel flag in this isn't associated with any of them, but rather with invoking this command that builds all of them.

I imagine your use case is something like this:

When building myapp, I'd like to always build it with deps and build all the targets in parallel. But I don't want this to affect other targets and other apps. It's a legitimate use case. Right now we allow you to set parallel and maxParallel for all commands, but you cannot set the defaults for some commands.

@LayZeeDK
Copy link
Contributor Author

@vsavkin

I imagine your use case is something like this:

When building myapp, I'd like to always build it with deps and build all the targets in parallel. But I don't want this to affect other targets and other apps. It's a legitimate use case. Right now we allow you to set parallel and maxParallel for all commands, but you cannot set the defaults for some commands.

Yes, we wanted to set withDeps and parallel for an application using the @nrwl/angular:webpack-browser executor for its build target when it has buildable workspace library dependencies which are using the @nrwl/angular:ng-packagr-lite executor for their build targets.

The schema of @nrwl/angular:webpack-browser doesn't allow the withDeps and parallel options to be set in workspace.json, so it seems that there's no way to ensure these options are always added to the nx build for the application project in question. However, if withDeps isn't added, it can result in an application build using stale dependencies.

@vsavkin vsavkin closed this Feb 25, 2021
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants