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.

Sign up for GitHub

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

Do not patch prototypes with render exposed only as a getter. #898

Merged
merged 3 commits into from
Jan 28, 2017
Merged

Do not patch prototypes with render exposed only as a getter. #898

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a00e414
Do not patch prototypes with render exposed only as a getter.
arunoda Jan 27, 2017
c0f89e7
Use Object.getOwnPropertyDescriptor to make things simpler.
arunoda Jan 27, 2017
68f1c78
Get the prototype which has the render method.
arunoda Jan 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions client/patch-react.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,23 @@ export default (handleError = () => {}) => {
prototype instanceof React.Component ||
prototype instanceof React.PureComponent

let dynamicWrapper = withWrapOwnRender

if (isClassComponent) {
if (prototype.render) {
prototype.render = wrapRender(prototype.render)
// Sometimes render method is created with only a getter.
// In that case we can't override it with a prototype. We need to
// do it dynamically.
if (canOverrideRender(prototype)) {
prototype.render = wrapRender(prototype.render)
} else {
dynamicWrapper = withWrapRenderAlways
}
}

// wrap the render method in runtime when the component initialized
// for class-properties.
Component = wrap(Component, withWrapOwnRender)
Component = wrap(Component, dynamicWrapper)
} else {
// stateless component
Component = wrapRender(Component)
Expand Down Expand Up @@ -73,6 +82,14 @@ export default (handleError = () => {}) => {
}
return result
}

function withWrapRenderAlways (fn, ...args) {
const result = fn.apply(this, args)
if (this.render) {
this.render = wrapRender(this.render)
}
return result
}
}

function wrap (fn, around) {
Expand All @@ -93,3 +110,10 @@ function wrap (fn, around) {

return _fn
}

function canOverrideRender (prototype) {
const descriptor = Object.getOwnPropertyDescriptor(prototype, 'render')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to walk up the prototype chain for a proper check, in case the component has been sub-classed, i.e. render() is defined on a super class (getOwnPropertyDescriptor only retrieves the descriptor for the immediate object). I'd also recommend to patch on exactly that prototype object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dbo aka the prototype master

if (!descriptor) return true

return descriptor.writable
}