Proposal: Audit PureComponent usage and potentially stop that

[BPScott]

Part of the post-v4 work is to update our examples to use hooks. @AndrewMusgrave made some fantastic progress on this and @amrocha has been reviewing it and he brought up something interesting: Pervasive usage of useCallback() makes them a bit crap and noisy to read.

Killing the useCallbacks would make for easier to read examples but there's a gotcha: The majority of our components use PureComponent and not using useCallback would mean they always fail their memoisation check (as you end up passing in a new function every time) - meaning a mechanism for supposed performance improvement actually becomes harmful as you always do more work with no chance of hitting the short-circuit path.

A component has two choices and we must be consistent if we want to showcase the correct way to use a component:

• Don't use React.PureComponent/React.memo in the component, and don't use useCallback to wrap functions passed into the component (as there is no shallow compare, it is preferable to avoid the small overhead of memoification)
• Use React.PureComponent/React.memo in the component and use useCallback to wrap functions passed into the component (as the wrapping allows the memoisation to work)

A lot of our components use PureComponent which make it seem like the second way is the right way to go but I'm not convinced that a most of our usage of PureComponent is actually justified.

---

A chat with @TheMallen in #web-foundation-tech suggests that their consensus is to avoid using React.PureComponent and React.memo until you know you need them and are sure there is a meaningful performance improvement thanks to profiling information. This approach to profiling and validating before jumping on a path with a different performance tradeoff is echoed by https://kentcdodds.com/blog/usememo-and-usecallback and https://reacttraining.com/blog/react-inline-functions-and-performance/ .

It looks like the overwhelming majority of our PureComponent usage is cargo-culty - we've slapped it onto components without ensuring it actually helps as many of the components in question are leaf nodes that often will have very little complex content inside them (e.g. Avatar, Banner, TextField).

I would like to take some time to audit our usage of PureComponent on all our components to validate our assumption that PureComponent makes a positive difference. If this turns out to be unfounded then we should remove PureComponent on the components that do not benefit from it.

// @cc @chloerice @dleroux @danrosenthal and @tmlayton for our earlier conversations on this

[BPScott]

This also has knock-on effects regarding our hookification work.

For instance consider the hookification of ActionMenu/components/ RollupActions/RollupActions.tsx.

It defines a handleRollupToggle function that then gets passed into Popover's onClose method. Should we use useCallback to wrap that toggle function or should we not bother?

Currently Popover is a PureComponent so we should use useCallback, but if our research suggests it shouldn't be memoised then we shouldn't use useCallback. We should need to make a call on that so we don't have to do any follow up work.

[tmlayton]

I'm not convinced that a most of our usage of PureComponent is actually justified
usage is cargo-culty

Yes, I believe this is the case as well. Of the components I’ve looked at which make use of PureComponent, it is/was used incorrectly.

[BPScott]

I've gone back and forth on this and I'm leaning towards "lets just always use useCallback" both in calling code and examples.

Yes it's a bit more noisy in the cases where you're creating a function that will be passed into a non-pure component but it's one less decision to make when coding compared to "go check if you're passing this into a PureComponent, if you are then use useCallback otherwise don't". It will also help insulate us against future changes when we decide we need to make a regular component be PureComponent or wrapped in React.memo -because all call sites should already using useCallback they're PureComponent-friendly, so we won't have to update all the call sites to not break the optimization as they're already playing nice.

Thus our hookification work isn't blocked by this as we shall always use useCallback anyway

[mbaumbach]

Potentially related, but React may be providing a solution in the future to the useCallback hook being used everywhere and it muddying the waters: https://gist.github.com/sebmarkbage/a5ef436427437a98408672108df01919. Specifically this part:

However, the recent heavy use of useMemo/useCallback in React has a similar effect. I have some ideas to auto-add useMemo/useCallback using a Babel compiler plugin so that you don't have to think about it same as how templating languages do it. But you always have the ability to do the memoization yourself too rather than being hidden in compiler magic. The nice thing about this approach is that it also use with external helpers that produce immutable values like the .filter(...) function above.

[BPScott]

Thanks for the link @mbaumbach, it's an interesting thought for the future. It sounds like such automation is a while away and waiting for that to appear won't help us though. In the past React have been pretty decent at codemods for migration paths - perhaps they'll provide a codemod that will identify explict usages of useCallback that could be removed and replaced by their babel plugin. I'd bet on us being explicit for now and there being a migration path in the future - we're not going to break anything by being explicit.

A line that resonated was:

This is interesting because our approach really stems from making code consistent. This sometimes means making it equally confusing in all cases rather than easy in common ones

Originally this was about how they handle stale closures but I think it applies to this decision too. I like the idea of pushing for consistency and that line the same ethos as why I'm leaning "lets just always use useCallback" both in calling code and examples.

[mbaumbach]

I think that's a good approach @BPScott. It's almost certainly going to be a straightforward codemod to remove any useCallback references should they release the Babel plugin. It's possible the Babel plugin wouldn't be entirely fitting for all consumers of Polaris either, so it's harder to rely on that as a library provider.

[ghost]

This issue has been inactive for 180 days and labeled with Icebox. It will be closed in 7 days if there is no further activity.

[BPScott]

still a thing

[BPScott]

More literature on this: https://royi-codes.now.sh/thousand-usecallbacks/

[ghost]

This issue has been inactive for 180 days and labeled with Icebox. It will be closed in 7 days if there is no further activity.

[BPScott]

Still a thing that should be addressed.

[tmlayton]

🍿 Sequel to iRobot where the war wages on, I present to you...iProbot

[BPScott]

Wherein Ben is disincentivized to open issues describing technical debt because they just get closed before by stalebot before being acted upon.

[IlanaB]

@BPScott is this something already actionable in web? We have a ton of usage of useCallback and useMemo there and I'm inclined to suggest that we start removing a bunch of these

issue for reference

[AndrewMusgrave]

is this something already actionable in web

I imagine so, useCallback & useMemo are very fragile. Even ignoring the argument of whether using the hooks could improve/degraded performance I'm sure there are quite a few places where props are changing every render & we're not receiving any benefit from the hook.

[BPScott]

Yep, what Andrew said. Though this ticket is more related to "cargo-culty use of React.memo() is overkill, so stop wrapping components in React.memo()" rather than "reduce usage of useCallback()/useMemo()"

[github-actions]

This issue seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action.
→ If there's no activity within a week, then a bot will automatically close this.
Thanks for helping to improve Shopify's design system and dev experience.

[BPScott]

Still a thing that I think is worth doing and would like to be done.

[github-actions]

Hi! We noticed there hasn’t been activity on this issue in a while. After 30 days, it will close automatically.

If it’s still relevant, or you have updates, comment and let us know. And don’t worry, you can always re-open later if needed.