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

Jump to bottom

Wrap render method created using class properties. #850

Closed
wants to merge 2 commits into from
Closed

Wrap render method created using class properties. #850

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b84c7cd
Wrap render method created using class properties.
arunoda Jan 21, 2017
0ed6fbe
Remove unwated codebase.
arunoda Jan 21, 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
17 changes: 14 additions & 3 deletions client/patch-react.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

// prototype. This happens when the render method created with class-properties.
// With this fix, we'll wrap the render method in runtime when the component initialized
const originalComponentWillMount = prototype.componentWillMount
prototype.componentWillMount = function (...args) {
if (originalComponentWillMount) {
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?

}
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.

} else {
// stateless component
Component = wrapRender(Component)
Expand All @@ -38,14 +50,14 @@ export default (handleError = () => {}) => {
return forceUpdate.apply(this, args)
}

function wrapRender (render) {
function wrapRender (render, context) {
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.

} catch (err) {
handleError(err)
return null
Expand All @@ -54,7 +66,6 @@ export default (handleError = () => {}) => {

// copy all properties
Object.assign(_render, render)

render.__wrapped = _render.__wrapped = _render

return _render
Expand Down