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(zone.js): only one listener should also throw an error #41868

Closed
wants to merge 1 commit into from

Conversation

JiaLiPassion
Copy link
Contributor

Close #41867

In the previous commit #41562 (comment),
the error thrown in the event listener will be caught and re-thrown, but there is a bug
in the commit, if there is only one listener for the specified event name, the error
will not be re-thrown, so this commit fixes the issue and make sure the error is re-thrown.

@google-cla google-cla bot added the cla: yes label Apr 29, 2021
@JiaLiPassion JiaLiPassion added area: zones target: patch This PR is targeted for the next patch release labels Apr 29, 2021
@ngbot ngbot bot modified the milestone: Backlog Apr 29, 2021
@jessicajaniuk
Copy link
Contributor

There seems to be a few legit failures in Google3 related to this change where we see this:

Error: Uncaught (in promise): Object: {"error":"unexpected error"}
and
Error: Uncaught (in promise): TypeError: Cannot read property 'pipe' of undefined

@JiaLiPassion
Copy link
Contributor Author

@jessicajaniuk, I updated the PR and keep the behavior compatible, could you re-run the presubmit? Thank you.

@jessicajaniuk
Copy link
Contributor

@JiaLiPassion We're unfortunately still seeing the exact same failing tests.

@jessicajaniuk
Copy link
Contributor

@JiaLiPassion I'll try to put a stackblitz together for you to see an example of the tests.

@JiaLiPassion
Copy link
Contributor Author

@jessicajaniuk , got it, thank you!

@br-star
Copy link

br-star commented May 3, 2021

Hi folks, how is the progress going on this?

@jessicajaniuk
Copy link
Contributor

@br-star We're seeing issues within Google3 with this change. So it's still being iterated on. There's also a big timezone difference between contributors. So we thank you for your patience.

@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit state: blocked labels May 3, 2021
@br-star
Copy link

br-star commented May 18, 2021

Any update? We're having to manually cherrypick the revert CL to our rapid prod branch as a workaround.

@jessicajaniuk
Copy link
Contributor

We talked off github, but just for visibility, @br-star is going to create a stackblitz for this issue.

@AndrewKushnir AndrewKushnir added the P2 The issue is important to a large percentage of users, with a workaround label May 20, 2021
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @JiaLiPassion. I've left a couple comments, could you please have a look when you get a chance?

We are also investigating the issues in Google's codebase related to this fix (it looks like some errors where not re-thrown and silently ignored) and will share more info soon.

packages/zone.js/lib/common/events.ts Show resolved Hide resolved
packages/zone.js/lib/common/events.ts Show resolved Hide resolved
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Reviewed-for: size-tracking

packages/zone.js/lib/common/events.ts Show resolved Hide resolved
Copy link
Member

@alxhub alxhub left a comment

Choose a reason for hiding this comment

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

This LGTM as a fix, though I second @AndrewKushnir's desire for some additional type info within the larger system here.

@pullapprove pullapprove bot requested a review from atscott June 1, 2021 23:43
@JiaLiPassion JiaLiPassion force-pushed the task-error branch 2 times, most recently from e8214ab to c6f745c Compare June 2, 2021 00:21
Close angular#41867

In the previous commit angular#41562 (comment),
the error thrown in the event listener will be caught and re-thrown, but there is a bug
in the commit, if there is only one listener for the specified event name, the error
will not be re-thrown, so this commit fixes the issue and make sure the error is re-thrown.
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Jun 9, 2021

Getting ready for landing this PR:

  • cleanup in Google's codebase is nearing completion
  • I've rebased this PR to rerun CI
  • new presubmit was started
  • global presubmit was also started

Thank you.

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Reviewed-for: size-tracking

@AndrewKushnir AndrewKushnir removed the request for review from mhevery June 9, 2021 21:22
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Reviewed-for: size-tracking

@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Jun 10, 2021
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

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

LGTM 🍪

Thanks, @JiaLiPassion, for your work on this.

reviewed-for: size-tracking

@AndrewKushnir AndrewKushnir added the action: merge The PR is ready for merge by the caretaker label Jun 10, 2021
alxhub pushed a commit that referenced this pull request Jun 10, 2021
…ly (#41868)

Close #41867

In the previous commit #41562 (comment),
the error thrown in the event listener will be caught and re-thrown, but there is a bug
in the commit, if there is only one listener for the specified event name, the error
will not be re-thrown, so this commit fixes the issue and make sure the error is re-thrown.

PR Close #41868
@alxhub alxhub closed this in 299f92c Jun 10, 2021
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Jun 10, 2021

Just a quick comment as a wrap up: this change was merged and synced into Google's codebase. Google's codebase required a sizable amount of cleanup work since the change that introduced the problem became available in g3 immediately after the sync (on Apr 21), thus there was a number of regressed targets. This problem does not seem to affect users externally since the latest ZoneJS package release (0.11.4) happened mid Feb, so the code that had the bug was not released publicly yet and the next ZoneJS release will have the fix as well.

Thanks everyone for helping with this fix! 👍

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: zones cla: yes P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Thrown error no longer propagates (caught in the try-catch inside invokeTask in patchEventTarget in events.ts)
6 participants