refactor(core): Async tagging zone dev by JiaLiPassion · Pull Request #47416 · angular/angular

JiaLiPassion · 2022-09-13T05:57:38Z

Remove zone-async-tagging implementation from zone.js and move the
implementation to @angular/core, so @angular/core can import this
package more easily for better treeshaking.
Add async tagging zone implemenation into @angular/core package.
So we don't need to get the AsyncStackTaggingZoneSpec from global
instance, we can import the class directly for better treeshaking.
Only load this ZoneSpec when ngDevMode is true.

dgp1130

Overall LGTM.

One implementation nit is to consider doing this in one commit so this change is very clearly a move from zone.js to @angular/core, instead of a delete and add across two commits.

Two trade offs are being made here which I want to be explicit about:

Moving AsyncStackTaggingZoneSpec into @angular/core means that other users of Zone can't get this debugging benefit. zone.js is experimental I think and not recommended or widely used outside of Angular, so maybe we're just ok with that.
This change presents a required runtime dependency on Zone.js in Angular dev builds, which might cause trouble with users who run Angular without Zones.
- AsyncStackTaggingZoneSpec will always be tree shaken in prod mode, so it's not an issue there, but in dev mode it will be retained.
- Technically @angular/core already has a required peer dep on zone.js according to package.json, but that's easy to ignore. With this change, I think it would be a hard error to not have a zone.js peer dep.
- Do we understand the runtime impact of importing AsyncStackTaggingZoneSpec without the rest of zone.js?
- If the runtime impact is a problem, then in google3 I think we'd need a patch to tree shake AsyncStackTaggingZoneSpec usage out for zoneless use cases. I'm less certain of our options in the CLI without moving the import to polyfills.ts or environments.ts.

As long as we are ok with 1. and can make sure zoneless apps work in a reasonable fashion for 2., then I'm good with this change.

JiaLiPassion · 2022-09-15T04:28:51Z

@dgp1130 , thank you for the review, I will make this PR into a single commit.

Yeah, I don't think there are a lot of zone.js user outside the Angular ecosystem, maybe it is ok.
I also considered about this one,

Since async-stack-tagging is just a class, it doesn't require zone.js, even zone.js is not loaded(such as noop zone case) it will still work without issues.
To use NgZone, https://github.com/angular/angular/blob/43cb391268f966cd4a02ee0ce903255e02533779/packages/core/src/zone/ng_zone.ts#L130, import zone.js is required, so add the AsyncStackTaggingZoneSpec will not add any additional requirement for zone.js.
And just like you mentioned, in prod mode, it will be tree shaken.

devversion

LGTM for dev-infra .bzl

dgp1130

SGTM @JiaLiPassion, thanks for clarifying the runtime impact for Zoneless users.

AndrewKushnir

LGTM, just a couple minor comments.

packages/core/src/zone/async-stack-tagging.ts

packages/core/src/zone/ng_zone.ts

AndrewKushnir

@JiaLiPassion thanks for addressing the feedback! Just left a tiny comment about the any type.

AndrewKushnir · 2022-09-16T21:48:31Z

packages/core/src/zone/async-stack-tagging.ts

@JiaLiPassion do we still need as any here?

@AndrewKushnir , thank you, forget this one, just updated and remove this any.

1. Remove `zone-async-tagging` implementation from zone.js and move the implementation to `@angular/core`, so `@angular/core` can import this package more easily for better treeshaking. 2. Add `async tagging zone` implemenation into `@angular/core` package. So we don't need to get the `AsyncStackTaggingZoneSpec` from `global` instance, we can import the `class` directly for better treeshaking. 3. Only load this ZoneSpec when `ngDevMode` is `true`.

pkozlowski-opensource

LGTM

Reviewed-for: size-tracking

jelbourn

LGTM

alxhub

Reviewed-for: global-approvers

alxhub · 2022-09-23T21:44:40Z

This PR was merged into the repository by commit 8637253.

) 1. Remove `zone-async-tagging` implementation from zone.js and move the implementation to `@angular/core`, so `@angular/core` can import this package more easily for better treeshaking. 2. Add `async tagging zone` implemenation into `@angular/core` package. So we don't need to get the `AsyncStackTaggingZoneSpec` from `global` instance, we can import the `class` directly for better treeshaking. 3. Only load this ZoneSpec when `ngDevMode` is `true`. PR Close angular#47416

angular-automatic-lock-bot · 2022-10-24T16:10:32Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

JiaLiPassion requested review from alan-agius4, clydin, dgp1130, jelbourn and mgechev September 13, 2022 05:57

pullapprove bot requested review from alxhub and devversion September 13, 2022 05:57

JiaLiPassion force-pushed the async-tagging-zone-dev branch from 4e239cf to a1e0e54 Compare September 13, 2022 06:06

JiaLiPassion added area: zones Issues related to zone.js target: major This PR is targeted for the next major release area: devtools labels Sep 13, 2022

ngbot bot modified the milestone: Backlog Sep 13, 2022

JiaLiPassion added target: minor This PR is targeted for the next minor release and removed target: major This PR is targeted for the next major release labels Sep 13, 2022

JiaLiPassion force-pushed the async-tagging-zone-dev branch from a1e0e54 to 43cb391 Compare September 13, 2022 13:16

dgp1130 reviewed Sep 13, 2022

View reviewed changes

JiaLiPassion force-pushed the async-tagging-zone-dev branch 2 times, most recently from 8c9510a to 64e08ba Compare September 15, 2022 06:15

alan-agius4 approved these changes Sep 15, 2022

View reviewed changes

JiaLiPassion requested a review from dgp1130 September 15, 2022 12:33

devversion approved these changes Sep 15, 2022

View reviewed changes

dgp1130 approved these changes Sep 15, 2022

View reviewed changes

jelbourn requested review from AndrewKushnir and removed request for alxhub September 15, 2022 22:23

AndrewKushnir approved these changes Sep 16, 2022

View reviewed changes

packages/core/src/zone/async-stack-tagging.ts Outdated Show resolved Hide resolved

packages/core/src/zone/ng_zone.ts Outdated Show resolved Hide resolved

pullapprove bot requested a review from alxhub September 16, 2022 00:11

JiaLiPassion force-pushed the async-tagging-zone-dev branch from 64e08ba to 1c349a6 Compare September 16, 2022 05:34

AndrewKushnir approved these changes Sep 16, 2022

View reviewed changes

JiaLiPassion force-pushed the async-tagging-zone-dev branch from 1c349a6 to ae0fabe Compare September 17, 2022 04:54

AndrewKushnir approved these changes Sep 20, 2022

View reviewed changes

JiaLiPassion added the action: merge The PR is ready for merge by the caretaker label Sep 20, 2022

pkozlowski-opensource added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: merge The PR is ready for merge by the caretaker labels Sep 20, 2022

JiaLiPassion requested a review from pkozlowski-opensource September 21, 2022 14:56

JiaLiPassion mentioned this pull request Sep 22, 2022

feat(@angular-devkit/schematics): update zone.js to 0.11.8 and add async tagging API support angular/angular-cli#23750

Closed

pkozlowski-opensource approved these changes Sep 23, 2022

View reviewed changes

clydin approved these changes Sep 23, 2022

View reviewed changes

jelbourn approved these changes Sep 23, 2022

View reviewed changes

mgechev approved these changes Sep 23, 2022

View reviewed changes

alxhub approved these changes Sep 23, 2022

View reviewed changes

alxhub added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 23, 2022

alxhub closed this in 8637253 Sep 23, 2022

angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 24, 2022

Comments

Conversation

JiaLiPassion commented Sep 13, 2022

Uh oh!

dgp1130 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JiaLiPassion commented Sep 15, 2022

Uh oh!

devversion left a comment

Choose a reason for hiding this comment

Uh oh!

dgp1130 left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AndrewKushnir left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir Sep 16, 2022

Choose a reason for hiding this comment

Uh oh!

JiaLiPassion Sep 17, 2022

Choose a reason for hiding this comment

Uh oh!

pkozlowski-opensource left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

alxhub left a comment

Choose a reason for hiding this comment

Uh oh!

alxhub commented Sep 23, 2022

Uh oh!

angular-automatic-lock-bot bot commented Oct 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

dgp1130 left a comment •

edited

Loading

AndrewKushnir left a comment •

edited

Loading

AndrewKushnir left a comment •

edited

Loading