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

JiaLiPassion · 2023-03-26T05:47:01Z

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.

packages/zone.js/test/browser/browser.spec.ts

DavidANeil

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.

packages/zone.js/test/browser/browser.spec.ts

packages/zone.js/lib/common/abort-controller.ts

packages/zone.js/lib/common/events.ts

DavidANeil · 2023-03-29T19:36:18Z

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.

physedo · 2023-07-12T05:47:57Z

@JiaLiPassion @dylhunn @AndrewKushnir ? Is this fixed?

jeripeierSBB · 2023-10-12T11:28:04Z

@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).

packages/zone.js/lib/common/abort-controller.ts

kyubisation · 2023-12-12T12:39:18Z

@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 · 2023-12-12T12:57:51Z

I will check this one this week, thanks.

kyubisation · 2023-12-12T12:59:08Z

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

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.

packages/zone.js/lib/common/events.ts

packages/zone.js/lib/common/fetch.ts

packages/zone.js/lib/common/events.ts

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

LGTM

alan-agius4 · 2023-12-13T14:45:57Z

TGP
TGP Deflake

jessicajaniuk · 2023-12-18T15:35:15Z

This PR was merged into the repository by commit 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

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. PR Close #49595

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

angular-automatic-lock-bot · 2024-01-18T00:09:20Z

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.}

…9595) 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. PR Close angular#49595

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 angular#49595

…9595) 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. PR Close angular#49595

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 angular#49595

…9595) 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. PR Close angular#49595

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 angular#49595

JiaLiPassion requested a review from AndrewKushnir March 26, 2023 05:47

DavidANeil reviewed Mar 28, 2023

View reviewed changes

packages/zone.js/test/browser/browser.spec.ts Outdated Show resolved Hide resolved

JiaLiPassion force-pushed the abort-signal branch from ff8c0b6 to 63daf54 Compare March 28, 2023 22:23

JiaLiPassion requested a review from DavidANeil March 28, 2023 22:24

JiaLiPassion force-pushed the abort-signal branch from 63daf54 to 3001611 Compare March 28, 2023 22:37

DavidANeil approved these changes Mar 29, 2023

View reviewed changes

packages/zone.js/test/browser/browser.spec.ts Outdated Show resolved Hide resolved

packages/zone.js/lib/common/abort-controller.ts Outdated Show resolved Hide resolved

packages/zone.js/lib/common/events.ts Outdated Show resolved Hide resolved

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

DavidANeil approved these changes Mar 29, 2023

View reviewed changes

dylhunn added the area: zones Issues related to zone.js label Apr 4, 2023

ngbot bot added this to the Backlog milestone Apr 4, 2023

DavidANeil reviewed Oct 14, 2023

View reviewed changes

packages/zone.js/lib/common/abort-controller.ts Outdated Show resolved Hide resolved

JiaLiPassion force-pushed the abort-signal branch from 8a51837 to 3d06423 Compare December 13, 2023 00:55

JiaLiPassion force-pushed the abort-signal branch from 3d06423 to da86d24 Compare December 13, 2023 01:55

JiaLiPassion requested review from DavidANeil and alan-agius4 December 13, 2023 02:47

alan-agius4 reviewed Dec 13, 2023

View reviewed changes

packages/zone.js/lib/common/events.ts Outdated Show resolved Hide resolved

alan-agius4 reviewed Dec 13, 2023

View reviewed changes

packages/zone.js/lib/common/events.ts Outdated Show resolved Hide resolved

DavidANeil approved these changes Dec 13, 2023

View reviewed changes

packages/zone.js/lib/common/events.ts Outdated Show resolved Hide resolved

packages/zone.js/lib/common/fetch.ts Outdated Show resolved Hide resolved

alan-agius4 reviewed Dec 13, 2023

View reviewed changes

packages/zone.js/lib/common/events.ts Outdated Show resolved Hide resolved

alan-agius4 reviewed Dec 13, 2023

View reviewed changes

packages/zone.js/lib/common/events.ts Outdated Show resolved Hide resolved

alan-agius4 added the action: review The PR is still awaiting reviews from at least one requested reviewer label Dec 13, 2023

alan-agius4 added the target: patch This PR is targeted for the next patch release label Dec 13, 2023

JiaLiPassion force-pushed the abort-signal branch from 1c4f3cc to 9a302a3 Compare December 13, 2023 12:15

JiaLiPassion force-pushed the abort-signal branch from 9a302a3 to eb5ef9a Compare December 13, 2023 12:21

JiaLiPassion requested a review from alan-agius4 December 13, 2023 12:45

alan-agius4 approved these changes Dec 13, 2023

View reviewed changes

alan-agius4 added the action: global presubmit The PR is in need of a google3 global presubmit label Dec 13, 2023

alan-agius4 removed the request for review from AndrewKushnir December 13, 2023 14:44

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 closed this in d4973ff Dec 18, 2023

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 18, 2024

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

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

Uh oh!

Conversation

JiaLiPassion commented Mar 26, 2023

Uh oh!

Uh oh!

DavidANeil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DavidANeil commented Mar 29, 2023

Uh oh!

physedo commented Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeripeierSBB commented Oct 12, 2023

Uh oh!

Uh oh!

kyubisation commented Dec 12, 2023

Uh oh!

JiaLiPassion commented Dec 12, 2023

Uh oh!

kyubisation commented Dec 12, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alan-agius4 left a comment

Choose a reason for hiding this comment

Uh oh!

alan-agius4 commented Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jessicajaniuk commented Dec 18, 2023

Uh oh!

angular-automatic-lock-bot bot commented Jan 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

physedo commented Jul 12, 2023 •

edited

Loading

alan-agius4 commented Dec 13, 2023 •

edited

Loading