GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — refactor(core): Async tagging zone dev by JiaLiPassion · Pull Request #47416 · angular/angular

Conversation

@JiaLiPassion

@JiaLiPassion JiaLiPassion commented Sep 13, 2022

Copy link
Copy Markdown
Contributor
  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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
AleksanderBodurri reacted with heart emoji
All reactions
  • ❤️ 1 reaction
@JiaLiPassion
JiaLiPassion requested review from alan-agius4, clydin, dgp1130, jelbourn and mgechev September 13, 2022 05:57
@pullapprove
pullapprove Bot requested review from alxhub and devversion September 13, 2022 05:57
@JiaLiPassion
JiaLiPassion force-pushed the async-tagging-zone-dev branch from 4e239cf to a1e0e54 Compare September 13, 2022 06:06
@JiaLiPassion 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 ngbot Bot modified the milestone: Backlog Sep 13, 2022
@JiaLiPassion 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
JiaLiPassion force-pushed the async-tagging-zone-dev branch from a1e0e54 to 43cb391 Compare September 13, 2022 13:16
dgp1130
dgp1130 reviewed Sep 13, 2022
View reviewed changes

@dgp1130 dgp1130 left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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:

  1. 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.
  2. 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@JiaLiPassion

JiaLiPassion commented Sep 15, 2022

Copy link
Copy Markdown
Contributor Author

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

  1. Yeah, I don't think there are a lot of zone.js user outside the Angular ecosystem, maybe it is ok.
  2. 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JiaLiPassion
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
alan-agius4 approved these changes Sep 15, 2022
View reviewed changes
@JiaLiPassion
JiaLiPassion requested a review from dgp1130 September 15, 2022 12:33
devversion
devversion approved these changes Sep 15, 2022
View reviewed changes

@devversion devversion left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM for dev-infra .bzl

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
dgp1130
dgp1130 approved these changes Sep 15, 2022
View reviewed changes

@dgp1130 dgp1130 left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jelbourn
jelbourn requested review from AndrewKushnir and removed request for alxhub September 15, 2022 22:23
AndrewKushnir
AndrewKushnir approved these changes Sep 16, 2022
View reviewed changes

@AndrewKushnir AndrewKushnir left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM, just a couple minor comments.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread packages/core/src/zone/async-stack-tagging.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread packages/core/src/zone/ng_zone.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.
@pullapprove
pullapprove Bot requested a review from alxhub September 16, 2022 00:11
@JiaLiPassion
JiaLiPassion force-pushed the async-tagging-zone-dev branch from 64e08ba to 1c349a6 Compare September 16, 2022 05:34
AndrewKushnir
AndrewKushnir approved these changes Sep 16, 2022
View reviewed changes

@AndrewKushnir AndrewKushnir left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread packages/core/src/zone/async-stack-tagging.ts Outdated

@AndrewKushnir AndrewKushnir Sep 16, 2022

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@JiaLiPassion do we still need as any here?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@JiaLiPassion JiaLiPassion Sep 17, 2022

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@JiaLiPassion
refactor(zone.js): remove zone-async-tagging from zone.js
ae0fabe 
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`.
@JiaLiPassion
JiaLiPassion force-pushed the async-tagging-zone-dev branch from 1c349a6 to ae0fabe Compare September 17, 2022 04:54
AndrewKushnir
AndrewKushnir approved these changes Sep 20, 2022
View reviewed changes
@JiaLiPassion JiaLiPassion added the action: merge The PR is ready for merge by the caretaker label Sep 20, 2022
@pkozlowski-opensource 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
JiaLiPassion requested a review from pkozlowski-opensource September 21, 2022 14:56
@JiaLiPassion 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
pkozlowski-opensource approved these changes Sep 23, 2022
View reviewed changes

@pkozlowski-opensource pkozlowski-opensource left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Reviewed-for: size-tracking

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
clydin
clydin approved these changes Sep 23, 2022
View reviewed changes
jelbourn
jelbourn approved these changes Sep 23, 2022
View reviewed changes

@jelbourn jelbourn left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mgechev
mgechev approved these changes Sep 23, 2022
View reviewed changes
alxhub
alxhub approved these changes Sep 23, 2022
View reviewed changes

@alxhub alxhub left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Reviewed-for: global-approvers

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@alxhub 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

alxhub commented Sep 23, 2022

Copy link
Copy Markdown
Member

This PR was merged into the repository by commit 8637253.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@alxhub alxhub closed this in 8637253 Sep 23, 2022
sonukapoor pushed a commit to sonukapoor/angular that referenced this pull request Sep 27, 2022
@JiaLiPassion @sonukapoor
refactor(zone.js): remove zone-async-tagging from zone.js (angular#47416
8579cd0 
)

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

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

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Oct 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mgechev mgechev mgechev approved these changes
@pkozlowski-opensource pkozlowski-opensource pkozlowski-opensource approved these changes
@alxhub alxhub alxhub approved these changes
@devversion devversion devversion approved these changes
@dgp1130 dgp1130 dgp1130 approved these changes
@alan-agius4 alan-agius4 alan-agius4 approved these changes
@clydin clydin clydin approved these changes
@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes
+1 more reviewer
@jelbourn jelbourn jelbourn approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: devtools area: zones Issues related to zone.js target: minor This PR is targeted for the next minor release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

10 participants

@JiaLiPassion @alxhub @mgechev @jelbourn @pkozlowski-opensource @devversion @dgp1130 @alan-agius4 @clydin @AndrewKushnir
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.