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): define tasks array as enumerable: false on element #33694

Closed
wants to merge 1 commit into from

Conversation

@sod
Copy link
Contributor

sod commented Nov 8, 2019

  • Bugfix

fixes #33692

@sod sod requested a review from angular/fw-zones as a code owner Nov 8, 2019
@googlebot googlebot added the cla: yes label Nov 8, 2019
@sod sod force-pushed the sod:issue-33692 branch from 65af5f3 to b7227c2 Nov 8, 2019
@sod sod force-pushed the sod:issue-33692 branch from b7227c2 to 51c8c71 Nov 8, 2019
@kara kara added the comp: zones label Nov 8, 2019
@ngbot ngbot bot added this to the needsTriage milestone Nov 8, 2019
@JiaLiPassion

This comment has been minimized.

Copy link
Contributor

JiaLiPassion commented Nov 13, 2019

@sod , thanks for the PR, but currently @angular/core depends on the eventListeners method (https://github.com/angular/angular/blob/1b8b04cf26401508cd171374e044e9510842e4f2/packages/core/src/debug/debug_node.ts), which is added by zone.js, and inside eventListeners implementaiton, it require those property to be enumerable.
The code of zone.js is here

const keys = Object.keys(target);
.
(Sorry I click close and comment button accidentally), maybe we can discuss this later or we can make some refactor on zone.js first.

@sod

This comment has been minimized.

Copy link
Contributor Author

sod commented Nov 13, 2019

Thanks for clarification.

Isn't there a security risk to expose event handlers globally accessible on elements? And why are the handlers spreaded onto the element? Doesn't this make the DOM megamorphic and thus code that works with the DOM slower? Why not use a weakmap to internally connect handlers & elements?

@JiaLiPassion

This comment has been minimized.

Copy link
Contributor

JiaLiPassion commented Nov 14, 2019

@sod, yeah, the purpose to attach handlers in such data structure is to increase the performance (especially memory GC), just like you said, maybe we can use some better design, we may discuss it here and make some refactor. Thank you!

@sod

This comment has been minimized.

Copy link
Contributor Author

sod commented Nov 14, 2019

ok :)

I'll close this PR then.

@sod sod closed this Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.