-
Notifications
You must be signed in to change notification settings - Fork 408
fix(closure): patchOnProperty with exact eventNames as possible #768
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to double check the event names and the target.
lib/common/utils.ts
Outdated
export function patchOnProperties(obj: any, properties: string[]) { | ||
if (properties) { | ||
for (let i = 0; i < properties.length; i++) { | ||
patchProperty(obj, 'on' + properties[i]); | ||
const validEventNames = getEventNames(obj, properties); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we have a fixed set rather than performing this complex operation on each startup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mhevery, yes, in my first commit, my goal is to make a fixed set, but it seems sometimes patch non exist property may cause errors, I will do more research and describe it more clear here.
I have modified the code to patch fixed set of event names.
And there is a potential risk (the risk also exists in the previous code), in some version of Browser, some property doesn't exists. for example,
in IE9, onprogress
don't exist in XMLHttpRequest's prototype, so the code will
return false in following code without zone.js patch.
'onprogress' in (new XMLHttpRequest());
after zone.js patch in this PR, the code above will return true.
In previous code, we patch XMLHttpRequest with patchOnProperties(XMLHttpRequest.prototype, null)
, so it will only patch the event names which exists in XMLHttpRequest.prototype, that's why I added the getEventNames
logic to make sure we only patch the properties exist in target.
5f6fd60
to
bdc88b0
Compare
lib/common/utils.ts
Outdated
@@ -60,6 +60,7 @@ export const isMix: boolean = typeof process !== 'undefined' && | |||
!!(typeof window !== 'undefined' && (window as any)['HTMLElement']); | |||
|
|||
export function patchProperty(obj: any, prop: string) { | |||
// TODO: @JiaLiPassion, if the desc not exists in obj, should we still patch the property? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so.
The way I imagine this to work is that we have a list of all properties, and than we only patch those properties, but only if the property already exist. But we don't look for extra ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mhevery , got it, thanks! I also think we should do like that, I will modify it and do some tests.
I have modified the code, patchProperty
will only patch the property which exist in obj
, please review.
2c59100
to
2852d35
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should patch window
object in IE correctly.
This has been fixed with passing Object.getPrototypeOf(window)
to patchProperty
method.
da11864
to
a787bf8
Compare
lib/browser/property-descriptor.ts
Outdated
@@ -23,24 +230,32 @@ export function propertyDescriptorPatch(_global: any) { | |||
if (canPatchViaPropertyDescriptor()) { | |||
// for browsers that we can patch the descriptor: Chrome & Firefox | |||
if (isBrowser) { | |||
patchOnProperties(window, eventNames.concat(['resize'])); | |||
patchOnProperties(window, eventNames, Object.getPrototypeOf(window)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that we are getting all of the properties of the window, does not seem right. Why can't we have a fixed list here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mhevery , the reason is in IE/Edge, the onProp
don't exist in window
but in WindowPrototype
.
The code below describe the issue.
const desc = Object.getOwnPropertyDescriptor(window, 'onload');
// in IE
console.log(desc); // will be undefined.
// in chrome/firefox
console.log(desc); // will be {configurable:true,get/set....}
So in new patchProperty
method, we only patch the prop which exist in the target object, so I pass the prototype of window
for the existency check.
https://github.com/JiaLiPassion/zone.js/blob/a787bf8ca6922465a6456c4182d7c61f46fb0f4f/lib/common/utils.ts#L73
export function patchProperty(obj: any, prop: string, prototype?: any) {
let desc = Object.getOwnPropertyDescriptor(obj, prop);
if (!desc && prototype) {
// when patch window object, use prototype to check prop exist or not
const prototypeDesc = Object.getOwnPropertyDescriptor(prototype, prop);
if (prototypeDesc) {
desc = {enumerable: true, configurable: true};
}
}
// if the descriptor not exists or is not configurable
// just return
if (!desc || !desc.configurable) {
return;
}
....
3bceebe
to
ee14819
Compare
related to #722.
We should try to patch onProperties with exact eventNames as possible.
in this PR, I separate the eventNames into several categories.
So we can patch each target with exact eventNames.
For example, we patch HTMLElement with
globalEventHandlers + element + webgl + form + details
,and we patch Window with
globalEventHandlers + window
.