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.

@kyubisation
Copy link
Contributor

kyubisation commented Dec 12, 2023

@JiaLiPassion Thank you very much. I apologize for being pushy.

@JiaLiPassion
fix(zone.js): support addEventListener with signal option.
da86d24 
Close angular#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.
alan-agius4
alan-agius4 reviewed Dec 13, 2023
View reviewed changes
alan-agius4
alan-agius4 reviewed Dec 13, 2023
View reviewed changes
DavidANeil
DavidANeil approved these changes Dec 13, 2023
View reviewed changes
alan-agius4
alan-agius4 reviewed Dec 13, 2023
View reviewed changes
alan-agius4
alan-agius4 reviewed Dec 13, 2023
View reviewed changes
@alan-agius4 alan-agius4 added the action: review The PR is still awaiting reviews from at least one requested reviewer label Dec 13, 2023
@alan-agius4 alan-agius4 added the target: patch This PR is targeted for the next patch release label Dec 13, 2023
@JiaLiPassion
fix(zone.js): handle fetch with AbortSignal
eb5ef9a 
fetch support AbortSignal, zone.js schedules a macroTask when fetch()

```
fetch(..., {signal: abortSignal});
```

we should also be able to cancel fetch with `zoneTask.cancel` call.
So this commit create an internal AbortSignal to handle
`zoneTask.cancel()` call and also delegate the `options.signal` from the
user code.
alan-agius4
alan-agius4 approved these changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

LGTM

@alan-agius4 alan-agius4 added the action: global presubmit The PR is in need of a google3 global presubmit label Dec 13, 2023
@alan-agius4 alan-agius4 removed the request for review from AndrewKushnir December 13, 2023 14:44
@alan-agius4
Copy link
Contributor

alan-agius4 commented Dec 13, 2023 •
edited
Loading

TGP
TGP Deflake

@alan-agius4 alan-agius4 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 action: global presubmit The PR is in need of a google3 global presubmit labels Dec 13, 2023
@jessicajaniuk
Copy link
Contributor

jessicajaniuk commented Dec 18, 2023

This PR was merged into the repository by commit b06b24b.

jessicajaniuk pushed a commit that referenced this pull request Dec 18, 2023
@JiaLiPassion @thePunderWoman
fix(zone.js): handle fetch with AbortSignal (#49595)
b06b24b 
fetch support AbortSignal, zone.js schedules a macroTask when fetch()

```
fetch(..., {signal: abortSignal});
```

we should also be able to cancel fetch with `zoneTask.cancel` call.
So this commit create an internal AbortSignal to handle
`zoneTask.cancel()` call and also delegate the `options.signal` from the
user code.

PR Close #49595
jessicajaniuk pushed a commit that referenced this pull request Dec 18, 2023