-
Notifications
You must be signed in to change notification settings - Fork 26k
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
Conversation
e7026e0
to
bb8b696
Compare
There seems to be a few legit failures in Google3 related to this change where we see this:
|
55f97c2
to
e10df0c
Compare
e10df0c
to
b82872c
Compare
@jessicajaniuk, I updated the PR and keep the behavior compatible, could you re-run the presubmit? Thank you. |
@JiaLiPassion We're unfortunately still seeing the exact same failing tests. |
@JiaLiPassion I'll try to put a stackblitz together for you to see an example of the tests. |
@jessicajaniuk , got it, thank you! |
Hi folks, how is the progress going on this? |
@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. |
Any update? We're having to manually cherrypick the revert CL to our rapid prod branch as a workaround. |
We talked off github, but just for visibility, @br-star is going to create a stackblitz for this issue. |
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.
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.
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: size-tracking
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.
This LGTM as a fix, though I second @AndrewKushnir's desire for some additional type info within the larger system here.
e8214ab
to
c6f745c
Compare
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.
Getting ready for landing this PR:
Thank you. |
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: size-tracking
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: size-tracking
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.
…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
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! 👍 |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
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.