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

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

Merged
merged 3 commits into from
Jan 28, 2017
Merged
Changes from all commits
Commits
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
46 changes: 42 additions & 4 deletions client/patch-react.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,24 +16,37 @@ export default (handleError = () => {}) => {

React.createElement = function (Component, ...rest) {
if (typeof Component === 'function') {
// We need to get the prototype which has the render method.
// It's possible to have render inside a deeper prototype due to
// class extending.
const prototypeWithRender = getRenderPrototype(Component)
const { prototype } = Component

// assumes it's a class component if render method exists.
const isClassComponent = Boolean(prototype && prototype.render) ||
const isClassComponent = Boolean(prototypeWithRender) ||
// subclass of React.Component or PureComponent with no render method.
// There's no render method in prototype
// when it's created with class-properties.
prototype instanceof React.Component ||
prototype instanceof React.PureComponent

let dynamicWrapper = withWrapOwnRender

if (isClassComponent) {
if (prototype.render) {
prototype.render = wrapRender(prototype.render)
if (prototypeWithRender) {
// 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(prototypeWithRender)) {
prototypeWithRender.render = wrapRender(prototypeWithRender.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 +86,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 +114,20 @@ function wrap (fn, around) {

return _fn
}

function getRenderPrototype (Component) {
let proto = Component.prototype

while (true) {
if (proto.hasOwnProperty('render')) return proto
proto = Object.getPrototypeOf(proto)
if (!proto) return null
}
}

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
}