Skip to content
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

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

JiaLiPassion
Copy link
Contributor

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

packages/zone.js/test/browser/browser.spec.ts 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 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

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.

@ngbot ngbot bot added this to the Backlog milestone Apr 4, 2023
@physedo
Copy link

physedo commented Jul 12, 2023 •
edited

@JiaLiPassion @dylhunn @AndrewKushnir ? Is this fixed?

@jeripeierSBB
Copy link
Contributor

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

@kyubisation
Copy link

@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

I will check this one this week, thanks.

@kyubisation
Copy link

@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.
DavidANeil
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 Show resolved Hide resolved
@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

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

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
@JiaLiPassion @thePunderWoman
fix(zone.js): support addEventListener with signal option. (#49595)
ded2bc9 
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
jessicajaniuk pushed a commit that referenced this pull request Dec 18, 2023
@JiaLiPassion @thePunderWoman
fix(zone.js): handle fetch with AbortSignal (#49595)
4a892a3 
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
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 18, 2024
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
@JiaLiPassion @ChellappanRajan
fix(zone.js): support addEventListener with signal option. (angular#4…
8776dfb 
…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
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
@JiaLiPassion @ChellappanRajan
fix(zone.js): handle fetch with AbortSignal (angular#49595)
4903adf 
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
rlmestre pushed a commit to rlmestre/angular that referenced this pull request Jan 26, 2024
@JiaLiPassion @rlmestre
fix(zone.js): support addEventListener with signal option. (angular#4…
e08fd20 
…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
rlmestre pushed a commit to rlmestre/angular that referenced this pull request Jan 26, 2024
@JiaLiPassion @rlmestre
fix(zone.js): handle fetch with AbortSignal (angular#49595)
7fc8313 
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
danieljancar pushed a commit to danieljancar/angular that referenced this pull request Jan 26, 2024
@JiaLiPassion @danieljancar
fix(zone.js): support addEventListener with signal option. (angular#4…
6435b86 
…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
danieljancar pushed a commit to danieljancar/angular that referenced this pull request Jan 26, 2024
@JiaLiPassion @danieljancar
fix(zone.js): handle fetch with AbortSignal (angular#49595)
03d3219 
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
amilamen pushed a commit to amilamen/angular that referenced this pull request Jan 26, 2024
@JiaLiPassion @amilamen
fix(zone.js): support addEventListener with signal option. (angular#4…
a7ec227 
…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
amilamen pushed a commit to amilamen/angular that referenced this pull request Jan 26, 2024
@JiaLiPassion @amilamen
fix(zone.js): handle fetch with AbortSignal (angular#49595)
1902d5c 
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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alan-agius4 alan-agius4 alan-agius4 approved these changes

@DavidANeil DavidANeil DavidANeil approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: zones target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

Zone.js does not properly handle AbortSignal in addEventListener
8 participants
@JiaLiPassion @DavidANeil @physedo @jeripeierSBB @kyubisation @alan-agius4 @jessicajaniuk @dylhunn