Sitelet https://web.archive.org/web/20221212203924/https://github.com/angular/angular/issues/47949
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

EventTarget patcher is incompatible with frozen EventTargets #47949

Open
eligrey opened this issue Nov 2, 2022 · 3 comments
Open

EventTarget patcher is incompatible with frozen EventTargets #47949

eligrey opened this issue Nov 2, 2022 · 3 comments
Assignees
Labels
area: zones P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Milestone

Comments

@eligrey
Copy link

eligrey commented Nov 2, 2022 •

Which @angular/* package(s) are the source of the bug?

zone.js

Is this a regression?

No

Description

I publish an API whose primary interface is a frozen EventTarget. A customer reached out to me with an error originating from Angular Zone.js (reduced testcase & error noted below) that is interfering with use of this EventTarget.

It is clear that this library is attempting to store state in EventTarget instance fields. This does not follow best practices and is inadvisable as EventTarget instances can be frozen.

Please update zone.js/lib/common/events.ts to use private variables to store state instead of mutating public fields on EventTarget instances.

Please provide a link to a minimal reproduction of the bug

JS input:

Object.freeze(new EventTarget).addEventListener('test', () => { /* ... */ });

Please provide the exception or error you saw

Error:

Uncaught TypeError: Cannot add property __zone_symbol__testfalse, object is not extensible

Please provide the environment you discovered this bug in (run ng version)

N/A

Anything else?

Testcase:

let pass = true;
try {
  Object.freeze(new EventTarget).addEventListener('test', () => {})
} catch (ex) {
  pass = false;
}
console.log('EventTarget patcher is compatible with frozen EventTargets', pass)
@eligrey eligrey changed the title EventTarget patcher incompatible with frozen EventTargets EventTarget patcher is incompatible with frozen EventTargets Nov 2, 2022
@ngbot ngbot bot added this to the needsTriage milestone Nov 2, 2022
@alxhub
Copy link
Contributor

alxhub commented Nov 15, 2022 •

@JiaLiPassion may be able to confirm, but I believe the monkey-patching performed by zone.js is fairly critical for its bookkeeping. It is not a design goal to support Object.freeze.

I suppose an alternative would be using a WeakMap for such bookkeeping instead, but we'd need to do significant benchmarking to understand the tradeoffs here.

@alxhub alxhub added the P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent label Nov 16, 2022
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Nov 16, 2022
@alxhub
Copy link
Contributor

alxhub commented Dec 9, 2022

Closing as we have no plans to support frozen EventTarget instances with zone.js at this time.

@eligrey
Copy link
Author

eligrey commented Dec 11, 2022 •

Our library actively bypasses your library by pinning cached EventTarget methods. We're constrained by file size so we are disappointed to have to work around your bugs with additional code.

Let me know when you eventually get to cleaning up this tech debt (if ever) so that we can remove these countermeasures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: zones P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Projects
None yet
Development

No branches or pull requests

4 participants