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.

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

fix(zone.js): do not mutate event listener options (may be readonly) #55796

Closed
wants to merge 1 commit into from

Conversation

arturovt
Copy link
Contributor

@arturovt arturovt commented May 14, 2024

Prior to this commit, event listener options were mutated directly, for example,
options.signal = undefined or options.once = false.

This issue arises in apps using third-party libraries where the responsibility lies
with the library provider. Some libraries, like WalletConnect, pass an abort controller
as addEventListener options. Because the abort controller has the signal property,
this is a valid case. Thus, mutating options would throw an error since signal
is a readonly property.

Even though zone.js is being deprecated as Angular moves towards zoneless change detection,
this fix is necessary for apps that still use zone.js and cannot migrate to zoneless change
detection because they rely on third-party libraries and are not responsible for the code
used in them.

Closes #54142

@arturovt arturovt marked this pull request as ready for review May 14, 2024 20:12
@pullapprove pullapprove bot requested a review from JiaLiPassion May 14, 2024 20:12
@arturovt arturovt force-pushed the fix/54142-zone-ac branch 3 times, most recently from 651aea4 to 2b8f010 Compare May 15, 2024 16:27
@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer area: zones requires: TGP This PR requires a passing TGP before merging is allowed labels May 17, 2024
@ngbot ngbot bot modified the milestone: Backlog May 17, 2024
@AndrewKushnir AndrewKushnir added the target: patch This PR is targeted for the next patch release label May 17, 2024
@pullapprove pullapprove bot added target: patch This PR is targeted for the next patch release and removed target: patch This PR is targeted for the next patch release requires: TGP This PR requires a passing TGP before merging is allowed labels May 17, 2024
Copy link
Contributor

@JiaLiPassion JiaLiPassion left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks.

@AndrewKushnir AndrewKushnir added requires: TGP This PR requires a passing TGP before merging is allowed and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels May 18, 2024
@pullapprove pullapprove bot removed requires: TGP This PR requires a passing TGP before merging is allowed labels May 18, 2024
@AndrewKushnir AndrewKushnir added the requires: TGP This PR requires a passing TGP before merging is allowed label May 19, 2024
@pullapprove pullapprove bot removed the requires: TGP This PR requires a passing TGP before merging is allowed label May 19, 2024
@AndrewKushnir AndrewKushnir added the action: global presubmit The PR is in need of a google3 global presubmit label May 19, 2024
@AndrewKushnir
Copy link
Contributor

Global Presubmit.

Prior to this commit, event listener options were mutated directly, for example,
`options.signal = undefined` or `options.once = false`.

This issue arises in apps using third-party libraries where the responsibility lies
with the library provider. Some libraries, like WalletConnect, pass an abort controller
as `addEventListener` options. Because the abort controller has the `signal` property,
this is a valid case. Thus, mutating options would throw an error since `signal`
is a readonly property.

Even though zone.js is being deprecated as Angular moves towards zoneless change detection,
this fix is necessary for apps that still use zone.js and cannot migrate to zoneless change
detection because they rely on third-party libraries and are not responsible for the code
used in them.

Closes angular#54142
@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: global presubmit The PR is in need of a google3 global presubmit labels May 20, 2024
@AndrewKushnir
Copy link
Contributor

Caretaker note: TGP is "green" (except for unrelated build breakages), this PR is ready for merge.

@dylhunn
Copy link
Contributor

dylhunn commented May 22, 2024

This PR was merged into the repository by commit 85c1719.

dylhunn pushed a commit that referenced this pull request May 22, 2024
…55796)

Prior to this commit, event listener options were mutated directly, for example,
`options.signal = undefined` or `options.once = false`.

This issue arises in apps using third-party libraries where the responsibility lies
with the library provider. Some libraries, like WalletConnect, pass an abort controller
as `addEventListener` options. Because the abort controller has the `signal` property,
this is a valid case. Thus, mutating options would throw an error since `signal`
is a readonly property.

Even though zone.js is being deprecated as Angular moves towards zoneless change detection,
this fix is necessary for apps that still use zone.js and cannot migrate to zoneless change
detection because they rely on third-party libraries and are not responsible for the code
used in them.

Closes #54142

PR Close #55796
@dylhunn dylhunn closed this in 85c1719 May 22, 2024
@arturovt arturovt deleted the fix/54142-zone-ac branch May 23, 2024 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: merge The PR is ready for merge by the caretaker area: zones merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AbortController error with walletconnect lib after zone.js 0.14.3 update
4 participants