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.

Sign up for GitHub

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

Jump to bottom

feat(angular): add support for a custom builder #4903

Merged
merged 1 commit into from May 11, 2021
Merged

feat(angular): add support for a custom builder #4903

merged 1 commit into from May 11, 2021

Conversation

Jordan-Hall
Copy link
Contributor

Ability to support incremental builds when using custom builders

Current Behavior

Currently @nrwl/angular:webpack-browser only supports angular builder @angular-devkit/build-angular. However, this prevents people to use incremental builder when using common builder such as @angular-builders/custom-webpack

Expected Behavior

Ability to use customBuilder to overwrite the default angular builder while still support incremental builds

Related decussion

#4896

@nx-cloud
Copy link

nx-cloud bot commented Feb 26, 2021 •
edited

Nx Cloud Report

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

📂 See all runs for this branch

Status Command
#000000 nx affected --target=build
#000000 nx affected --target=e2e
#000000 nx affected --target=lint
#000000 nx affected --target=test
#000000 nx run-many --target=build --projects=angular
#000000 nx run-many --target=build-base --projects=angular
#000000 nx run-many --target=build-base --projects=cypress
#000000 nx run-many --target=build-base --projects=devkit
#000000 nx run-many --target=build-base --projects=eslint-plugin-nx
#000000 nx run-many --target=build-base --projects=jest
#000000 nx run-many --target=build-base --projects=linter
#000000 nx run-many --target=build-base --projects=storybook
#000000 nx run-many --target=build-base --projects=tao
#000000 nx run-many --target=build-base --projects=workspace
#000000 nx run-many --target=e2e --projects=e2e-angular
#000000 nx run-many --target=lint --projects=angular
#000000 nx run-many --target=test --projects=angular

Sent with 💌 from NxCloud.

@Jordan-Hall Jordan-Hall force-pushed the angular-custom-bulder-support branch 2 times, most recently from b0e95ab to a852a0e Compare February 26, 2021 02:26
@Jordan-Hall Jordan-Hall marked this pull request as ready for review February 26, 2021 02:38
@Jordan-Hall
Copy link
Contributor Author

Jordan-Hall commented Feb 28, 2021 •
edited

I think I figured out a better way of doing this. @vsavkin may be looking at the angular one at least. It's just a wrapper around the angular builder. Maybe add a setting. nx.json which would automatically wrap around any build inside workspace.json or angular.json (that's not part of nrwl builders). That way its not as opinionated but you can enable incremental builds.

@juristr
Copy link
Member

juristr commented Mar 12, 2021

@Jordan-Hall thx, I'm going to have a look

@Jordan-Hall
Copy link
Contributor Author

Jordan-Hall commented Mar 12, 2021 •
edited

No problem. Happy to produce a prototype on using the nx.json and ensuring any builder can be wrapped by a setting, rather than on builders itself.

@juristr juristr added the scope: angular Issues related to Angular support in Nx label May 7, 2021
@Coly010 Coly010 self-requested a review May 7, 2021 13:22
Coly010
Coly010 requested changes May 7, 2021
View reviewed changes
Copy link
Contributor

@Coly010 Coly010 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! 🔥

I've requested a change to the naming of the property to allow it represent the property more accurately.

As another point, however, how does this work with configurations?

Have you tested whether the third party builder receives the configuration object correctly?

For example, an nx build --prod should then pass the production configuration object right down to the third party builder.

packages/angular/src/builders/webpack-browser/schema.json Outdated Show resolved Hide resolved
@juristr juristr assigned Coly010 and unassigned juristr May 7, 2021
@juristr juristr removed their request for review May 7, 2021 14:38
@Jordan-Hall
Copy link
Contributor Author

This looks great! 🔥

I've requested a change to the naming of the property to allow it represent the property more accurately.

As another point, however, how does this work with configurations?

Have you tested whether the third party builder receives the configuration object correctly?

For example, an nx build --prod should then pass the production configuration object right down to the third party builder.

I'll make the change later today. Yes I've tested this and didn't have any issues.

@Jordan-Hall Jordan-Hall force-pushed the angular-custom-bulder-support branch from ffafffd to a255372 Compare May 8, 2021 18:34
@vercel
Copy link

vercel bot commented May 8, 2021 •
edited

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nrwl/nx-dev/madebCXeSHvEHJzNZw9pVYxEeiaX
✅ Preview: Canceled

[Deployment for 6b8ac39 canceled]

@Jordan-Hall Jordan-Hall force-pushed the angular-custom-bulder-support branch from a255372 to 1eab792 Compare May 8, 2021 18:58
@Jordan-Hall
feat(angular): feat(angular): add support for a target Builder
6b8ac39 
Ability to support incremental builds when using target builders

ISSUES CLOSED: nrwl#4896
@Jordan-Hall Jordan-Hall force-pushed the angular-custom-bulder-support branch from 1eab792 to 6b8ac39 Compare May 8, 2021 19:07
Coly010
Coly010 approved these changes May 10, 2021
View reviewed changes
Copy link
Contributor

@Coly010 Coly010 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! This looks great!

LGTM

@Coly010 Coly010 mentioned this pull request May 10, 2021
Merged
@Coly010 Coly010 merged commit a78fe65 into nrwl:master May 11, 2021
Coly010 added a commit to Coly010/nx that referenced this pull request May 17, 2021
@Coly010
Revert "feat(angular): add support for a target Builder (nrwl#4903)"
b943590 
This reverts commit a78fe65.
Coly010 added a commit that referenced this pull request May 18, 2021
@Coly010
Revert "feat(angular): add support for a target Builder (#4903)" (#5684)
fa01e90 
* Revert "feat(angular): add support for a target Builder (#4903)"

This reverts commit a78fe65.

* chore(angular): disable failing test
@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 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Coly010 Coly010 Coly010 approved these changes

Assignees

@Coly010 Coly010

Labels
scope: angular Issues related to Angular support in Nx
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Jordan-Hall @juristr @Coly010