EventTarget patcher is incompatible with frozen EventTargets

[eligrey]

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 = false;
try {
  Object.freeze(new EventTarget).addEventListener('test', () => {});
  pass = true;
} catch (ex) {}
console.log('EventTarget patcher is compatible with frozen EventTargets', pass)

[alxhub]

@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]

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

[eligrey]

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 fix this issue so that we can remove these countermeasures.

[JiaLiPassion]

I will work on it after the https://github.com/angular/angular/pull/49204 refactor PR is ok, so I can store the state to somewhere else instead of the global objects themselves.

[eligrey]

Thank you @JiaLiPassion. I appreciate that you're working to fix this!

[arturovt]

@eligrey Even though we could provide a fix for that issue, and technically, I think it's more approachable to store tasks attached to an event target separately in a weak map, as Alex pointed out, rather than creating new properties on the event target itself. Unfortunately, that fix would be breaking and could cause a number of failures in Google's codebase.

Given that this issue has no duplicates or upvotes, it might not be prioritized for investigation in the near future.