[charts] Replace react-spring with d3-transition/CSS transitions

[bernardobelchior]

• I have followed (at least) the PR section of the contributing guide.

[github-actions]

Thanks for adding a type label to the PR! 👍

[mui-bot]

Deploy preview: https://deploy-preview-16945--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against fd36408

[codspeed-hq]

CodSpeed Performance Report

Merging #16945 will improve performances by 12.91%

_{Comparing bernardobelchior:d3-animation-poc (7815904) with master (586a1c2)}

Summary

⚡ 1 improvements
✅ 6 untouched benchmarks

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
⚡	LineChartPro with big data amount	149.5 ms	132.4 ms	+12.91%

[JCQuintas]

Why is this state necessary?

[bernardobelchior]

I need the useLayoutEffect to run whenever the path element changes because the select call depends on the element. If I don't re-run the layout effect in that case, then the transition could apply to an element that isn't the current one.

This isn't likely to happen often, but it might, e.g.,:

function AnimatedLine(props) {
  // New component on every render
  const Line = ({ d }) => <path d={d} />
  const animateRef = useAnimatePath(props);

  return <Line ref={animateRef} d={props.d} />
};

Do you see a better option for this use case without using state?

[JCQuintas]

Couldn't you depend on the ref's current value then for the effect?

[bernardobelchior]

Do you mean something like this?

useEffect(() => {}, [ref.current])

[JCQuintas]

Yes

[bernardobelchior]

Now that I take a look at it, using state doesn't work for the case where a new component is created on every render as that would create an infinite loop.

But using a ref doesn't work either as I get this lint warning:

ESLint: Ref values (the current property) may not be accessed during render. (https://react.dev/reference/react/useRef) (react-compiler/react-compiler)

I guess I'll have to use a ref callback. I'll see if that's doable.

[JCQuintas]

Why leave it outside? This doesn't add any value to the user.

[bernardobelchior]

Yeah, I can move it inside, I guess. Just need to think of a good API

[alexfauquette]

The skip modification should clean the useLayoutEffect and call the interupt. Then nothing to do except propagating the prop.

So I assume it's not that much of an issue

[bernardobelchior]

This PR is slightly outdated. The new one is this one where I'm trying to migrate components one by one.

[alexfauquette]

Would it make sens that the hook removes those props

const propsWithAnimation = useAnimateBarProps(props)

with propsWithAnimation being the props from which we:

• If animation is active:
  • remove the animated props (the x, y, width, height)
  • add ref
• If animation is disabled
  • propagate the props
  • add the ref

I think a big plus of this strategy is that we can remove all those notion of animated.xxx and let use pass whatever x or y props to the component if they want to reuse it.
And if they want to use other animation, they can re-write the component.

[bernardobelchior]

I think a big plus of this strategy is that we can remove all those notion of animated.xxx and let use pass whatever x or y props to the component if they want to reuse it.

We can remove the react-spring animated props even without using the pattern you're suggesting, can't we? Or am I misunderstanding something?

I think we can return the props depending on skipAnimation, but I fear that will make types harder to handle, and I'm not seeing which other benefits there might be.

[alexfauquette]

Sorry, I had multiple thought at the same time and mixed them together

My initial point

What I had in mind when reviewing the PR was the discussion about being more flexible to let user reuse those hooks.

I was wondering why react-spring need to have both spring hooks and animated() components, which are a mess because of the spring types.

From what I understand, it's because the animated elements need to do some cleaning between the style properties that are spring values (and need to be properly handled), versus the one that are just JS values and can be passed directly to the component.

Since we only need to animate specific components, having hooks like useAnimateBarProps that indicates "This hook manage the x, y, width, and height of the element with ref. Don't care about those".

Conclusion, exporting only animation hooks per components seems to be a promising approach.

The props cleaning proposal

It's not a mandatory change, but more a discussion about what woudlbe a nice DX.

The question I have in mind was "How to let the user understand they should not use or modify the props the hooks is taking care?" And the answer I came with is "remove the props". But if we don't export the hook at first step, it's not urgent at all to have a perfect DX. as long as it works t's Ok 👍

[bernardobelchior]

Conclusion, exporting only animation hooks per components seems to be a promising approach.

Agreed 👍

How to let the user understand they should not use or modify the props the hooks is taking care?

That is a good question. The way the hook works at the moment, if a prop that it's animating is passed to the component, it will be overwritten, so I don't think there's a risk of the animation not working. However, there's a risk of surprise in case the passed a prop and it wouldn't be reflected in the element.

Removing props would solve some of the issues of this last point, but not all. For example, in the PieArc component, we need to animate props that are different from the attribute set in the element:

  const ref = usePieArcAnimatedProps({
    startAngle,
    endAngle,
    cornerRadius,
    paddingAngle,
    innerRadius,
    outerRadius,
  });

The attribute that is changed is d, but the props do not contain that.

Removing the props

Passing and returning props would work, but I don't think we could remove the ones set outside React's lifecycle as we still need to set them for SSR. See the SSR section below for the why (TLDR: useLayoutEffect doesn't work on the server).

However, the API is cleaner, at least in this simple example:

function PieArc(props: PieArcProps) {
  const animatedProps = usePieArcAnimatedProps(props);

  return (
    <PieArcRoot
      {...animatedProps}
    />
  );
}

It also solves the issue I identify below, so it could be an interesting approach. I'll explore it.

Room for improvement: Server-side rendering

One thing I don't like about the current solution is that usePieArcAnimatedProps hides the conversion of props to a path, but we still need to do it for the component's prop:

function PieArc(props: PieArcProps) {
  const {
    cornerRadius,
    endAngle,
    innerRadius,
    outerRadius,
    paddingAngle,
    startAngle,
  } = props;

  const ref = usePieArcAnimatedProps({
    startAngle,
    endAngle,
    cornerRadius,
    paddingAngle,
    innerRadius,
    outerRadius,
  });

  return (
    <PieArcRoot
      ref={ref}
      d={
        d3Arc().cornerRadius(cornerRadius)({
          padAngle: paddingAngle,
          startAngle,
          endAngle,
          innerRadius,
          outerRadius,
        })!
      }
    />
  );
}

We need to set d as a normal prop as well because useLayoutEffect doesn't run on the server, so we need to set it so that it's sent in the HTML.

Ideally, we'd provide a utility to convert the props to a path somehow.

[bernardobelchior]

Another option is mixed approach where only the spreadable props are provided. Example here.

If we accept all props and return the needed ones, I suppose it would be something like this.

To be honest, I don't love passing all those props to a hook because I don't know what the hook will do with them. It isn't clear which props might be changed and which ones won't be altered.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.