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

Update jsx-no-bind.md #1231

Merged
merged 1 commit into from May 31, 2017
Merged

Update jsx-no-bind.md #1231

merged 1 commit into from May 31, 2017

Conversation

vonovak
Copy link
Contributor

@vonovak vonovak commented May 30, 2017

No description provided.

@@ -1,6 +1,6 @@
# No `.bind()` or Arrow Functions in JSX Props (react/jsx-no-bind)

A `bind` call or [arrow function](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions) in a JSX prop will create a brand new function on every single render. This is bad for performance, as it will result in the garbage collector being invoked way more than is necessary.
A `bind` call or [arrow function](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions) in a JSX prop will create a brand new function on every single render. This is bad for performance, as it will result in the garbage collector being invoked way more than is necessary. It may also cause unnecessary re-renders if a brand new function is passed as a prop to a component that uses reference equality check on the prop to determine if it should update.
Copy link
Member

Choose a reason for hiding this comment

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

We should probably also add ", which includes arrow functions assigned in class properties" or something similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ljharb not quite sure what you mean here. Having the arrow function assigned as a class property is the recommended approach.

Copy link
Member

Choose a reason for hiding this comment

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

It most certainly is not the recommended approach.

That said, it does avoid the creation of functions in the render path, so you're right that it doesn't belong here.

Copy link
Contributor Author

@vonovak vonovak May 31, 2017

Choose a reason for hiding this comment

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

** recommended approach if one needs to preserve the this of a component, which is not always needed. Or is that otherwise? Yes, as for rendering, arrow functions that are not created in render won't trigger a re-render so this description should be fine.

Copy link
Member

Choose a reason for hiding this comment

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

The recommended approach is a constructor-bound instance method, so the meat of the function can be optimized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ljharb what do you mean by "the meat of the function can be optimized."? Can you point me to some resource that explains this?

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a resource to point to; I just mean that the bulk of the code can live on a shared prototype method, that is thus invoked many times (and shared across all instances), so that can get optimized - then the only part that's "one per instance" is the bind-proxy. With the class property approach, your function (which has, say, 6 lines of code) gets recreated for every instance, so the engine can't optimize those 6 lines - since every instance has a distinct copy of those 6 lines of code.

@ljharb ljharb merged commit 2748421 into jsx-eslint:master May 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants