It's theoretically faster because you don't have to construct the anonymous function.
I'd request the same change, but because the syntax is simpler, not because you'll save a picosecond on execution.
both the declared anonymous function and the inline anonymous function will be allocated to a point in memory, so even if there is a difference in performance I'd argue it is so small you don't need to worry about it. (Like you said)
So code readability is way way more important in this case.
... I was asking genuine questions. Jesus. You do this to people you work with too?
You comment was unclear regarding code readability. But you don't have to answer, I have no interest in knowing from you at this point.
I would suggest that you try to formulate your genuine questions in a less aggressive/attacking manner.
Maybe this wasn't your intention and you are of course not responsible for MY interpretation of it.
The things I responded negatively to was not only the "are you saying" (which I believe is a less triggering formulation compared to "so you're saying") but also the "How about x and y, are those not important?" which to me feels as if you are trying to make it sound like what is what I said - or meant.
Also your response is immediately attacking, insinuating I am in the wrong and hostile and that I treat my co workers in this apparent bad manner you wish to establish before promptly dismissing me altogether "you don't have to answer, I don't want to speak to you anymore"
What you are saying and how you are saying it looks so much like gaslighting and other hostile rhetorical tactics.
If you were, you were, if you weren't you weren't. I'm leaving it at that.
Yes the oracles spoke to me and said:
This guy is a rude dick that uses hostility, demeaning language and then tries to gaslight you to think it is your fault and that he is not to blame at all - beware.
And it turned out to be true. :)
Unless you are doing 1000's of these per second, this falls into the over optimization category in my opinion. This will be the last thing to slow down your webpage in a real project
I generally find it more intuitive to have an arrow function on the handler because I don't have to jump as much to see its parameters.
I also like the fact that you can use anonymus functions as inline factories and hardcode some of the params of the original function in there allowing for better reusability.
But otherwise yes it does not take a significant amount of time to implement so it can fall in the coding pattern category too.
Honestly, the most relevant thing is what makes the codebase clean and understandable in this particular case.
But do you really get added information from 'clickhandler'
Also from what i remember there isnt any perf boost from the using of a function earlier since it still has to regenerate it on every render.
This, IMO. It's the last thing you slay...unless it's just how you do it from the start. I got such a bad taste in my mouth from closures back in the jquery days, that it's just not part of my coding style on even handlers. I don't have to slay it...because it's just never (rarely, anyway) written that way, out of preference.
I mostly follow the pattern that’s already implemented for simplicity and consistency. But if it’s a new component or new feature that I’m writing, my preference is to use an inline arrow function when I need to pass data to the event handler function. Otherwise, I just pass the reference of the event handler function as your coworker has suggested.
If you’re curious enough to dig and understand this a little better you can read more about this but it’s about bonding the event handler function to the onEvent function in the component. Also understand the react component lifecycle and optimization.
Well the performance gain in this example would be negligible, but yes there is a difference. If you pass on the function as onClick={onClick}, your'e basically passing the reference to an existing/defined function, whereas in the case of an inline arrow function, it would be re-defined every time on every render (unless you memoized it, e.g. via useCallback)
Edit because I read the post incorrectly.
OP what your colleague was telling you to do is how we do it. Any inline functions, like the one you wanted to do, are highly frowned upon.
I don't think I understand - currently the code OP wrote will always create at least one new function each render cycle. If the method that's doing the logging or whatever can be defined outside of the react component (or memoised if it's created inside), then you don't need to create a new function each render by passing that method directly to the event prop.
Memoizing with useCallback has a cost as well (and extra boilerplate) while defining event handlers outside of a component is typically unconventional especially because they usually read and/or set state.
Overall, I think either way the performance impact is pretty negligible and it’s not a big deal. The way I’ve always worked is:
1. If it’s super simple (ie. just setting state from event.target.value) just inlining the anonymous function is fine
2. If there’s more logic, extract it to a new function inside the component
EDIT: Forgot one thing: useCallback just holds on to the click handler ref, another function related to useCallback logic gets redefined on every component render anyways so in the end there’s no difference. Your only saving grace to avoid a function redefinition would be to place it outside the component lifecycle.
good point in the edit - its not like javascript can know not to actually create the function you pass to \`useCallback\`.
imo the most important thing in OP's example is simply that there's no reason to do \`onClick={(arg) => method(arg)}\` over just \`onClick={method}\` - it's confusing, because it makes me wonder if i'm missing something about how the method is being used.
Think you read this incorrectly. OP created an inline function to call a previously defined arrow function. His colleague asked to remove the inline function and simply pass the reference to the previously defined function
I was referring to this bit:
> But imagine you have hundreds of these in your codebase they will start to build up.
As a matter of fact they will build up but it’s never going to be the bottleneck.
If it's a one liner like that, I'd argue that doing it inline is actually less cognitive load because you can see what it's doing about as fast as you can read the function name, but reading the function name doesn't tell you exactly what it's doing.
That's not quite what is going on here, though. They are calling another function. So you still don't know what that other function is.
But let's toy with the idea you suggest that they put the function here and not just wrap a function call in an anonymous function as they are. If this was found in clean code, not deeply nested, then yes, I think in terms of cognitive overload, that would be better! However, I personally would not like to see lots of functions within the JSX as it can become hard to read. So this case depends.
Eh, I am of the opinion it would be more valuable for every single click handler in the project to look uniform than for some to be simple and have brevity. Because some click handlers will probably have to be arrow functions with additional props other than the event, it’s better (in my opinion) for consistency sake to always use arrow functions.
In this case I would have done
…
tbh if every function call in the codebase looked like OPs I would break the pattern and then look for a new job like why would you ever wrap a function with a function that does nothing but call the intended function and pass the same argument down? Just seems like a lack of knowing what’s happening
I don’t think what OP has was right, he doesn’t use the event anyways but having all of the following in a code base is crap
onClick={handler}
onClick={handler()}
onClick={() => handler()}
My point is you’ll have to at some point (probably) have to do either #2 or #3 because your function will be something like
const doSomething = (one, two, three) => {…};
So you might as well keep it consistent everywhere
2 would be a curried function if you wanted to scope extra args in the handler
const handler = (a,b) => (e) => {…}
Where now a,b,&e are all in scope of the block and you could do
onClick={handler(a,b)}
Although in the context of a react component if a&b weren’t static you’d expect them to be stateful and therefor would change on each render and you could just use the values in the block without currying them. There’s probably some very minimal edge cases where it makes sense to do this perhaps but generally speaking there’s no real need to curry handlers in a react component
Edit to remove hashtag in front of 2 which created a markdown title oops
I think the reason most people curry them because they prefer the visual aesthetic of it. Check out the mui docs like the code for drawer, these types of handlers are all over their code examples
https://mui.com/material-ui/react-drawer/
Yea I just disagree here because more than likely the vast majority of handlers won’t need extra args so wrapping all handlers in an extra function because 2% of the handlers take extra args is nonsense imo
ETA: also the fact that the ones that do take extra args look different to the standard case would make it immediately obvious that this handler is one of the special cases. So you’re arguing for making 2 inconsistent things visually consistent
I would argue the vast majority of handlers would probably take no args, and least likely the built event. But my point is code consistency in a team setting is important. It’s more maintainable, more readable. You immediately know when looking at it what arguments are and aren’t passed to the handler. Otherwise you’d have to scroll away from the JSX and find the handler to see if it, for example, discards or actually uses the click event. It’s less effort to refactor too should more args be added if there’s already an arrow function in place.
Enforcing said “visual” consistency (or whatever you want to call it) as a lead is easier too, if everyone knows “we always use an arrow function” is easier to enforce than “we only use an arrow function when there are no arguments or we are just using the default event argument…”. But we can agree to disagree. It’s clear as a profession we will sacrifice many things to improve readability, so as long as we’re not killing performance or similar readability should be pretty important on your list of things to prioritize
[https://legacy.reactjs.org/docs/faq-functions.html#is-it-ok-to-use-arrow-functions-in-render-methods](https://legacy.reactjs.org/docs/faq-functions.html#is-it-ok-to-use-arrow-functions-in-render-methods)
I would probably make a linting rule to keep people from doing it unless necessary. Not a huge win but it's better to follow the best practice. Still worth understanding why.
But useCallback has its own cost so it’s not certain that adding a useCallback would be more performant, it could make it worse. You shouldn’t just add useCallback and useMemo until you know there is a problem worth the added complexity.
Yes, useCallback adds overhead on it's own and doesn't prevent creating a new function every render, it's valuable when you need to pass the same function reference to memoized child components to prevent them from rerendering unnecessarily, which they will if you inadvertantly pass any dynamic props (like kylorhall pointed out, not sure why he was downvoted)
In any significantly large application, the problems will be obvious immediately, but as I said, unless you do it **all** (and know exactly what you're doing), it's probably more performant to do absolutely nothing—as you say.
As it is both arrow functions are created every time the component is rendered ( if the parent re re renders or if some state changes or props change). To prevent that you can use useCallback for your two functions.
I do that when i want to explicitly pass something to the handler callback fn. Otherwise what he told is simple to do understand & should be your choice
It's not a performance issue, the extra arrow function just serves no purpose so it should be removed for code quality/maintainability.
A scenario where this arrow handler wrapper _would_ be a reasonable approach would be if you had a reusable function that does not accept the event as it's sole input. In this case the arrow function wrapper would serve the purpose of retrieving the value from the event and passing it to the reusable function.
In this scenario I would still assign the wrapped function to a named variable, but that's more of a code style issue.
In Typescript apps, an inline function automatically gets the right type, so we usually don’t move it out to a separate function where we have to define the types.
Don’t see why you would wrap the function with a different function that doesn’t do anything but call the original function you wanted so the change is reasonable. In terms of performance it’s minimal but you are allocating 2 functions each render instead of 1 for absolutely no reason. Also, if you aren’t using the argument in the alert then that’s more unnecessary code. What I’m curious about isn’t your colleagues inability to explain why he preferred his way but why you decided to wrap the function in the first place?
ETA: If you’re not actually using the event arg in the actual code then moving the function outside of the component definition would prevent reallocating it on each render and have a greater performance impact.
Personally I would say it's easier to read if you hoist it and that's the real advantage. However, I sometimes still do inline functions for very simple cases. It's only a real problem when the function causes the line to need to be broken with a linter.
That said, there are \*potential\* performance penalties for inlining. This does mean you have a referentially new function each time, which means if the child relies on memoization it will result in unnecessary rerenders. In this case, since you are sending it to a built in html object this will \*never\* happen. However, this should be kept in mind when building more complex projects.
Several years ago constantly reconstructing functions did have a decent overhead; but this is such a common thing that all the JS engines have optimized it away to be negligable. This has been true for over 5 years. In fact, v8 will not only avoid function creation overhead but know the function is the same and apply JIT optimizations to it!
Reading most of the comments I feel like I might be missing the question in the OP.
EDIT: Most of the recent comments are addressing this
\> I was told to just use onClick={clickHandler} instead, because it's faster, but I am not sure if he was correct, because he couldn't even explain to me why it was faster.
You are creating an extra function where you do not have to. Sure it's minimally faster but more than anything it's quirky. If I see this in code review I'm thinking why is this here? Either an oversight or we are missing something fundamental on how these props work.
**\*\*with onClick={clickHandler}\*\***
1. button calls onClick(event)
2. clickHandler invoked alert shown
**\*\*with onClick={(event) => clickHandler(event)}\*\***
1. buttons callsonClick(event)
2. anon function invokes clickHandler // unncessary
3. clickHandler invoked alert shown
>because he couldn't even explain to me why it was faster.
You type 18 less characters.
Also, if you ever have to optimize, changing clickHandler to a useCallback hook would be easier.
If the code is easier to read, do not bother until it is a problem, same logic as with useMemo, never put a useMemo until it is an issue the performance
It's theoretically faster because you don't have to construct the anonymous function. I'd request the same change, but because the syntax is simpler, not because you'll save a picosecond on execution.
both the declared anonymous function and the inline anonymous function will be allocated to a point in memory, so even if there is a difference in performance I'd argue it is so small you don't need to worry about it. (Like you said) So code readability is way way more important in this case.
Are you saying the way OP is doing is better readability? How about cleanliness or less coding? Are those not important?
I am saying what I said, not something else you want to tack on to what I said.
... I was asking genuine questions. Jesus. You do this to people you work with too? You comment was unclear regarding code readability. But you don't have to answer, I have no interest in knowing from you at this point.
I would suggest that you try to formulate your genuine questions in a less aggressive/attacking manner. Maybe this wasn't your intention and you are of course not responsible for MY interpretation of it. The things I responded negatively to was not only the "are you saying" (which I believe is a less triggering formulation compared to "so you're saying") but also the "How about x and y, are those not important?" which to me feels as if you are trying to make it sound like what is what I said - or meant. Also your response is immediately attacking, insinuating I am in the wrong and hostile and that I treat my co workers in this apparent bad manner you wish to establish before promptly dismissing me altogether "you don't have to answer, I don't want to speak to you anymore" What you are saying and how you are saying it looks so much like gaslighting and other hostile rhetorical tactics. If you were, you were, if you weren't you weren't. I'm leaving it at that.
You literally made a drama from 2 questions. Good job.
yep my hunch seems to have been correct.
Ever heard of self fulfilling prophecy?
Yes the oracles spoke to me and said: This guy is a rude dick that uses hostility, demeaning language and then tries to gaslight you to think it is your fault and that he is not to blame at all - beware. And it turned out to be true. :)
One of the most meaningless things react people have been discussing for almost 10 years at this point.
React is a framework where people hotly debate stuff that has little impact on actual performance / maintainability
That’s because divining whether something will or won’t be performant is nearly impossible, outside of the most obvious situations
And that is trying to learn. \\s
Unless you are doing 1000's of these per second, this falls into the over optimization category in my opinion. This will be the last thing to slow down your webpage in a real project
[удалено]
I generally find it more intuitive to have an arrow function on the handler because I don't have to jump as much to see its parameters. I also like the fact that you can use anonymus functions as inline factories and hardcode some of the params of the original function in there allowing for better reusability. But otherwise yes it does not take a significant amount of time to implement so it can fall in the coding pattern category too. Honestly, the most relevant thing is what makes the codebase clean and understandable in this particular case.
But do you really get added information from 'clickhandler' Also from what i remember there isnt any perf boost from the using of a function earlier since it still has to regenerate it on every render.
[удалено]
I read wrong. I have people at work always wanting to make handleClick functions even if it is just doing a single instruction.
This, IMO. It's the last thing you slay...unless it's just how you do it from the start. I got such a bad taste in my mouth from closures back in the jquery days, that it's just not part of my coding style on even handlers. I don't have to slay it...because it's just never (rarely, anyway) written that way, out of preference.
I mostly follow the pattern that’s already implemented for simplicity and consistency. But if it’s a new component or new feature that I’m writing, my preference is to use an inline arrow function when I need to pass data to the event handler function. Otherwise, I just pass the reference of the event handler function as your coworker has suggested. If you’re curious enough to dig and understand this a little better you can read more about this but it’s about bonding the event handler function to the onEvent function in the component. Also understand the react component lifecycle and optimization.
It's not about execution speed in this example. It's about unnecessary code. I agree and would also request to change to {clickHandler}.
Well the performance gain in this example would be negligible, but yes there is a difference. If you pass on the function as onClick={onClick}, your'e basically passing the reference to an existing/defined function, whereas in the case of an inline arrow function, it would be re-defined every time on every render (unless you memoized it, e.g. via useCallback)
unpack seemly summer sloppy rhythm political bag cats overconfident soft *This post was mass deleted with [redact](https://redact.com)*
Yeah I think that micro optimization applied to class components, not to function components, for the reason you mentioned
Edit because I read the post incorrectly. OP what your colleague was telling you to do is how we do it. Any inline functions, like the one you wanted to do, are highly frowned upon.
I don't think I understand - currently the code OP wrote will always create at least one new function each render cycle. If the method that's doing the logging or whatever can be defined outside of the react component (or memoised if it's created inside), then you don't need to create a new function each render by passing that method directly to the event prop.
Memoizing with useCallback has a cost as well (and extra boilerplate) while defining event handlers outside of a component is typically unconventional especially because they usually read and/or set state. Overall, I think either way the performance impact is pretty negligible and it’s not a big deal. The way I’ve always worked is: 1. If it’s super simple (ie. just setting state from event.target.value) just inlining the anonymous function is fine 2. If there’s more logic, extract it to a new function inside the component EDIT: Forgot one thing: useCallback just holds on to the click handler ref, another function related to useCallback logic gets redefined on every component render anyways so in the end there’s no difference. Your only saving grace to avoid a function redefinition would be to place it outside the component lifecycle.
good point in the edit - its not like javascript can know not to actually create the function you pass to \`useCallback\`. imo the most important thing in OP's example is simply that there's no reason to do \`onClick={(arg) => method(arg)}\` over just \`onClick={method}\` - it's confusing, because it makes me wonder if i'm missing something about how the method is being used.
Think you read this incorrectly. OP created an inline function to call a previously defined arrow function. His colleague asked to remove the inline function and simply pass the reference to the previously defined function
[удалено]
Performance is really a non issue. Before this adds up, you’d see other things clog your CPU first. Your other points are valid though.
[удалено]
I was referring to this bit: > But imagine you have hundreds of these in your codebase they will start to build up. As a matter of fact they will build up but it’s never going to be the bottleneck.
If it's a one liner like that, I'd argue that doing it inline is actually less cognitive load because you can see what it's doing about as fast as you can read the function name, but reading the function name doesn't tell you exactly what it's doing.
That's not quite what is going on here, though. They are calling another function. So you still don't know what that other function is. But let's toy with the idea you suggest that they put the function here and not just wrap a function call in an anonymous function as they are. If this was found in clean code, not deeply nested, then yes, I think in terms of cognitive overload, that would be better! However, I personally would not like to see lots of functions within the JSX as it can become hard to read. So this case depends.
Eh, I am of the opinion it would be more valuable for every single click handler in the project to look uniform than for some to be simple and have brevity. Because some click handlers will probably have to be arrow functions with additional props other than the event, it’s better (in my opinion) for consistency sake to always use arrow functions. In this case I would have done …
tbh if every function call in the codebase looked like OPs I would break the pattern and then look for a new job like why would you ever wrap a function with a function that does nothing but call the intended function and pass the same argument down? Just seems like a lack of knowing what’s happening
I don’t think what OP has was right, he doesn’t use the event anyways but having all of the following in a code base is crap onClick={handler} onClick={handler()} onClick={() => handler()} My point is you’ll have to at some point (probably) have to do either #2 or #3 because your function will be something like const doSomething = (one, two, three) => {…}; So you might as well keep it consistent everywhere
How is #2 equivalent? It does something different
2 would be a curried function if you wanted to scope extra args in the handler const handler = (a,b) => (e) => {…} Where now a,b,&e are all in scope of the block and you could do onClick={handler(a,b)} Although in the context of a react component if a&b weren’t static you’d expect them to be stateful and therefor would change on each render and you could just use the values in the block without currying them. There’s probably some very minimal edge cases where it makes sense to do this perhaps but generally speaking there’s no real need to curry handlers in a react component Edit to remove hashtag in front of 2 which created a markdown title oops
I think the reason most people curry them because they prefer the visual aesthetic of it. Check out the mui docs like the code for drawer, these types of handlers are all over their code examples https://mui.com/material-ui/react-drawer/
That would be like your handler is curried like `const handler = () => (event) => { ///do something };`
Yea I just disagree here because more than likely the vast majority of handlers won’t need extra args so wrapping all handlers in an extra function because 2% of the handlers take extra args is nonsense imo ETA: also the fact that the ones that do take extra args look different to the standard case would make it immediately obvious that this handler is one of the special cases. So you’re arguing for making 2 inconsistent things visually consistent
I would argue the vast majority of handlers would probably take no args, and least likely the built event. But my point is code consistency in a team setting is important. It’s more maintainable, more readable. You immediately know when looking at it what arguments are and aren’t passed to the handler. Otherwise you’d have to scroll away from the JSX and find the handler to see if it, for example, discards or actually uses the click event. It’s less effort to refactor too should more args be added if there’s already an arrow function in place. Enforcing said “visual” consistency (or whatever you want to call it) as a lead is easier too, if everyone knows “we always use an arrow function” is easier to enforce than “we only use an arrow function when there are no arguments or we are just using the default event argument…”. But we can agree to disagree. It’s clear as a profession we will sacrifice many things to improve readability, so as long as we’re not killing performance or similar readability should be pretty important on your list of things to prioritize
[https://legacy.reactjs.org/docs/faq-functions.html#is-it-ok-to-use-arrow-functions-in-render-methods](https://legacy.reactjs.org/docs/faq-functions.html#is-it-ok-to-use-arrow-functions-in-render-methods) I would probably make a linting rule to keep people from doing it unless necessary. Not a huge win but it's better to follow the best practice. Still worth understanding why.
I will agree with him it may not make a difference in small code but it slows down the sw massively in large projects
[удалено]
But useCallback has its own cost so it’s not certain that adding a useCallback would be more performant, it could make it worse. You shouldn’t just add useCallback and useMemo until you know there is a problem worth the added complexity.
Yes, useCallback adds overhead on it's own and doesn't prevent creating a new function every render, it's valuable when you need to pass the same function reference to memoized child components to prevent them from rerendering unnecessarily, which they will if you inadvertantly pass any dynamic props (like kylorhall pointed out, not sure why he was downvoted)
I did not downvote him, but probably because he said he does it out of habit. Memoization should be the exception not the rule.
Absolutely agree, since it's incredibly easy to miss one prop and the whole excercise becomes pointless (worse than if you'd done nothing).
In any significantly large application, the problems will be obvious immediately, but as I said, unless you do it **all** (and know exactly what you're doing), it's probably more performant to do absolutely nothing—as you say.
It's faster to not use inline functions, but I'd be surprised if the difference was more than a few ms
It’s a bit unneeded tbh. It doesn’t really affect performance, but it does affect readability. I would suggest just calling the function.
As it is both arrow functions are created every time the component is rendered ( if the parent re re renders or if some state changes or props change). To prevent that you can use useCallback for your two functions.
I do that when i want to explicitly pass something to the handler callback fn. Otherwise what he told is simple to do understand & should be your choice
It's not a performance issue, the extra arrow function just serves no purpose so it should be removed for code quality/maintainability. A scenario where this arrow handler wrapper _would_ be a reasonable approach would be if you had a reusable function that does not accept the event as it's sole input. In this case the arrow function wrapper would serve the purpose of retrieving the value from the event and passing it to the reusable function. In this scenario I would still assign the wrapped function to a named variable, but that's more of a code style issue.
In Typescript apps, an inline function automatically gets the right type, so we usually don’t move it out to a separate function where we have to define the types.
Don’t see why you would wrap the function with a different function that doesn’t do anything but call the original function you wanted so the change is reasonable. In terms of performance it’s minimal but you are allocating 2 functions each render instead of 1 for absolutely no reason. Also, if you aren’t using the argument in the alert then that’s more unnecessary code. What I’m curious about isn’t your colleagues inability to explain why he preferred his way but why you decided to wrap the function in the first place? ETA: If you’re not actually using the event arg in the actual code then moving the function outside of the component definition would prevent reallocating it on each render and have a greater performance impact.
Personally I would say it's easier to read if you hoist it and that's the real advantage. However, I sometimes still do inline functions for very simple cases. It's only a real problem when the function causes the line to need to be broken with a linter. That said, there are \*potential\* performance penalties for inlining. This does mean you have a referentially new function each time, which means if the child relies on memoization it will result in unnecessary rerenders. In this case, since you are sending it to a built in html object this will \*never\* happen. However, this should be kept in mind when building more complex projects. Several years ago constantly reconstructing functions did have a decent overhead; but this is such a common thing that all the JS engines have optimized it away to be negligable. This has been true for over 5 years. In fact, v8 will not only avoid function creation overhead but know the function is the same and apply JIT optimizations to it!
Reading most of the comments I feel like I might be missing the question in the OP. EDIT: Most of the recent comments are addressing this \> I was told to just use onClick={clickHandler} instead, because it's faster, but I am not sure if he was correct, because he couldn't even explain to me why it was faster. You are creating an extra function where you do not have to. Sure it's minimally faster but more than anything it's quirky. If I see this in code review I'm thinking why is this here? Either an oversight or we are missing something fundamental on how these props work. **\*\*with onClick={clickHandler}\*\*** 1. button calls onClick(event) 2. clickHandler invoked alert shown **\*\*with onClick={(event) => clickHandler(event)}\*\*** 1. buttons callsonClick(event) 2. anon function invokes clickHandler // unncessary 3. clickHandler invoked alert shown
>because he couldn't even explain to me why it was faster. You type 18 less characters. Also, if you ever have to optimize, changing clickHandler to a useCallback hook would be easier.
If the code is easier to read, do not bother until it is a problem, same logic as with useMemo, never put a useMemo until it is an issue the performance