fix(Button): memoize call to merge.all

[joshblack]

Reference: https://github.com/github/primer/issues/1191

This PR updates ButtonBase to memoize the call to merge.all with useMemo. This seemed like the best next step when I was taking a look at the issue, let me know if I misunderstood the approach!

A quick aside, I removed the default value of sx so that useMemo wouldn't be called each render. The fallback value has been moved to the merge.all call

Merge checklist

• Added/updated tests
• Added/updated documentation
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge

[changeset-bot]

🦋 Changeset detected

Latest commit: 9506801

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	76.31 KB (+0.03% 🔺)
dist/browser.umd.js	76.92 KB (+0.04% 🔺)

[siddharthkp]

Definitely an improvement! Would be helpful to run the same or similar benchmarks from https://github.com/github/primer/issues/1191

(without any data) I wonder if we'll get a lot of benefit from cache hits in memo when if it includes user provided props.sx. For example, two instances of the same small-primary-button but with different margins in sx would need to call the function again. We can memo the other three functions without sx if that helps.

Alternative solution: How do you feel about inserting css for all variants on render and using then a data-attribute to set the style. This would remove the function calls for getSomethingStyles on updates. We'll still have to merge it with sx, but that should be a less expensive call.

rendered to something like

<button className="btn-h4sh" data-variant="primary" data-size="small"/>

// css:
.btn-h4sh {
  // common and default styles
}

.btn-h4sh[data-variant=primary] {
  // primary styles
} 

We use data-attributes for styling in some other places like ButtonCounter and ActionList divider

[joshblack]

Thanks so much @siddharthkp!

I wonder if we'll get a lot of benefit from cache hits in memo when if it includes user provided props.sx

Great point, I moved it out of useMemo and now just always do sx={merge(merge(sxStyles, sxProp as SxProp))}

Would be helpful to run the same or similar benchmarks from https://github.com/github/primer/issues/1191

Sounds great, in order to profile would you recommend pulling down their project / npm link'ing primer and running the same trace?

How do you feel about inserting css for all variants on render and using then a data-attribute to set the style.

Happy to do it, would that be what you'd recommend?

[joshblack]

cc @pksjce if you have any recommendations for setting a baseline here and profiling different approaches!

[pksjce]

So there's a storybook perf addon currently on primer-react storybooks. Does that help in any way at all?

[joshblack]

@pksjce definitely! I'll use that for some quick ad-hoc measurements locally to see what you all think 👀

Scenarios

Baseline

• Use the Watch Counter Button story
• Interaction: click first button ten times
• Collect 10 copies at 10 samples each

useMemo (sxProp in dep array)

Code

const ButtonBase = forwardRef<HTMLElement, ButtonProps>(
  ({children, as: Component = 'button', sx: sxProp, ...props}, forwardedRef): JSX.Element => {
    const {leadingIcon: LeadingIcon, trailingIcon: TrailingIcon, variant = 'default', size = 'medium'} = props
    const {theme} = useTheme()
    const iconWrapStyles = {
      display: 'inline-block'
    }
    const sxStyles = useMemo(() => {
      return merge.all([
        getButtonStyles(theme),
        getSizeStyles(size, variant, false),
        getVariantStyles(variant, theme),
        (sxProp || {}) as SxProp
      ])
    }, [theme, size, variant, sxProp])
    return (
      <StyledButton as={Component} sx={sxStyles} {...props} ref={forwardedRef}>

useMemo (merge at sx call site)

Code

    const sxStyles = useMemo(() => {
      return merge.all([getButtonStyles(theme), getSizeStyles(size, variant, false), getVariantStyles(variant, theme)])
    }, [theme, size, variant])
    return (
      <StyledButton as={Component} sx={merge(sxStyles, sxProp as SxProp)} {...props} ref={forwardedRef}>

Summary

• In general, the test runs had a high amount of variation between them. I tried to run each scenario around 3x before reporting the measurement
• In either useMemo situation, the performance of the interaction decreased by roughly 15% (when sx was not present)
• In general, it seemed preferable to not call merge as the sx call site

Include sx prop in story

I also repeated this when adding in an {} as the value to the sx prop in each button in the story

Baseline

useMemo (sxProp in dep array)

useMemo (merge at sx call site)

Takeaways

I would say that there wasn't anything too conclusive with the initial perf audits of these different approaches. It seems like what wins out isn't by much.

There does seem to be some improvement in using useMemo. It also seems like if adding in sx is common then shifting it over to the prop site (sx={merge(sxStyles, sxProp)}) would net the biggest gain in updates.

[pksjce]

Thanks for that analysis!

Yes, so as I understand we want to memoize the internal style calculations and merge the user styles at the call site.
But ideally, it would be really good if we never made all these style calculations at every render of the button. I feel like its a combination of

• styled-components dynamic styles (can only be fixed by using ssr compatible styling library)
  FWIW this helps IconButton inherit from the ButtonBase and helps us support multitude of sizes and variants in a dev friendly way.
• us pulling theme variables dynamically ( if theme variables were available as css variables, that would fix this)

Fixing these two issues would fundamentally improve perf of Button component?