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.