-
Notifications
You must be signed in to change notification settings - Fork 78
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
Fixing a problem with auto-bind and multiple subclasses #348
Conversation
@@ -52,15 +52,38 @@ function isDomMeta(meta: any): meta is Base { | |||
return Boolean(meta.afterRender); | |||
} | |||
|
|||
const IGNORE_LIST: (string | symbol)[] = ['constructor', 'render']; | |||
const IGNORE_LIST: (string | symbol)[] = ['render', ...Object.getOwnPropertyNames(Object.getPrototypeOf({}))]; |
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.
We can exclude all the properties that come from Object.prototype
. Technically we might not even need this since we break our loop on subclasses of WidgetBase
?
p = Object.getPrototypeOf(p); | ||
} | ||
|
||
keys = keys.filter((k) => typeof instance[k] === 'function' && IGNORE_LIST.indexOf(k) === -1); |
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 filtering of keys through the ignore list is now done up here rather than down yonder.
src/widget-core/WidgetBase.ts
Outdated
if (autoBindCache.has(prototype)) { | ||
keys = autoBindCache.get(prototype) as string[]; | ||
} else { | ||
let p = prototype; |
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.
Don't need to do this, just use prototype
?
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.
@rorticus Did you see this? ^
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.
Removed extra variable p
!
@rorticus It also looks like CI is failing |
Type: bug
The following has been addressed in the PR:
prettier
as per the readme code style guidelinesDescription:
There is a problem with the new auto-bind code when there are multiple levels of inheritance. It currently only applies the binding logic to the first level of inheritance.
To fix this, we're walking the prototype chain up to
WidgetBase
and gathering all the properties from there. Since this is a bit of an expensive operation, we're also caching the result of the property gathering so it only needs to happen once per widget class.