Skip to content

fix(zone.js): support addEventListener with signal option. #49595

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
Closed
JiaLiPassion wants to merge 2 commits into from
Closed

fix(zone.js): support addEventListener with signal option. #49595

JiaLiPassion wants to merge 2 commits into from

Conversation

@JiaLiPassion
Copy link
Contributor

@JiaLiPassion JiaLiPassion commented Mar 26, 2023

Close #49591

const ac = new AbortController();
addEventListener(eventName, handler, {signal: ac.signal);`
ac.abort();

Currently zone.js doesn't support the signal option, this PR allows the user to use AbortContoller to remove the event listener.

DavidANeil
DavidANeil reviewed Mar 28, 2023
View reviewed changes
@JiaLiPassion JiaLiPassion requested a review from DavidANeil March 28, 2023 22:24
DavidANeil
DavidANeil approved these changes Mar 29, 2023
View reviewed changes
Copy link
Contributor

@DavidANeil DavidANeil left a comment

Choose a reason for hiding this comment

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

I think this design isn't satisfactory (if this were merged, I would probably patch it back to my "just backoff entirely when a signal is passed in" mode).

But I think it is better than the status quo, so I am approving for the sake of incremental improvement.

@JiaLiPassion JiaLiPassion force-pushed the abort-signal branch 3 times, most recently from 1a45a55 to 8a51837 Compare March 29, 2023 04:23
@DavidANeil
Copy link
Contributor

DavidANeil commented Mar 29, 2023

I believe you've fixed the memory leaks, so this is looking good.

I'm still not sure if something can be done about AbortSignal.timeout, though it can be done later in another PR.

@dylhunn dylhunn added the area: zones Issues related to zone.js label Apr 4, 2023
@ngbot ngbot bot added this to the Backlog milestone Apr 4, 2023
@physedo
Copy link

physedo commented Jul 12, 2023 •
edited
Loading

@JiaLiPassion @dylhunn @AndrewKushnir ? Is this fixed?

@jeripeierSBB
Copy link
Contributor

jeripeierSBB commented Oct 12, 2023

@JiaLiPassion @DavidANeil Would there be anything where I can support this PR?
We would like to use a lib which uses AbortController in their implementation and we cannot use it together with an Angular application (as long as it uses zone.js).

DavidANeil
DavidANeil reviewed Oct 14, 2023
View reviewed changes
@kyubisation
Copy link
Contributor

kyubisation commented Dec 12, 2023

@JiaLiPassion @DavidANeil As @jeripeierSBB mentioned; Is there any way to help? With the wider usage of AbortController in the web, the lack of support in zone.js is an intransparent error for consumers, that have no idea why specific code fails.

@JiaLiPassion
Copy link
Contributor Author

JiaLiPassion commented Dec 12, 2023

I will check this one this week, thanks.