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

Wrap render method created using class properties. #850

Closed
wants to merge 2 commits into from

Conversation

arunoda
Copy link
Contributor

@arunoda arunoda commented Jan 21, 2017

Fixes #828

With this, we are wrapping the render method added using ES2016+ class properties.
It doesn't add the render method to class's prototype. So, that's why we need to do this.

@arunoda arunoda changed the title Wrap render method created using class properties. [WIP] Wrap render method created using class properties. Jan 21, 2017
if (render.__wrapped) {
return render.__wrapped
}

const _render = function (...args) {
try {
return render.apply(this, args)
return render.apply(context || this, args)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why we need context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. We don't need this.

}

this.render = wrapRender(this.render, this)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should redefine constructor instead of using componentDidMount ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm quite not sure about that. This constructor function is something generated by babel. I just used that to detect a React component.

originalComponentWillMount.apply(this, args)
}

this.render = wrapRender(this.render, this)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should do this only when this.render didn't come from prototype.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's how it works. If there's a render method, it'll get wrapped by the above if block.

Did you mean something else?

@@ -19,6 +19,18 @@ export default (handleError = () => {}) => {
const { prototype } = Component
if (prototype && prototype.render) {
prototype.render = wrapRender(prototype.render)
} else if (prototype && prototype.constructor) {
// Still a React component instance, but there's no render method in
Copy link
Contributor

Choose a reason for hiding this comment

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

btw there may be render but might override it on constructor.

class Foo extends React.Component {
  render () {}
}
class Bar extends Foo {
  render = () => {};
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we do this on the componentWillMount. I assume this is fine.
Isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

For now, we don't monkeypatch componentWillMount if prototype.render exists.

@arunoda arunoda changed the title [WIP] Wrap render method created using class properties. Wrap render method created using class properties. Jan 21, 2017
@nkzawa
Copy link
Contributor

nkzawa commented Jan 22, 2017

alternate PR #856

@nkzawa nkzawa closed this Jan 22, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants