Refactor Components to use Hooks

[Xiot]

There is a desire to refactor the basic building blocks of react-vis into hooks.
This should clean up the code, and make sharing functionality, which is currently done via inheritance, much easier.

I think we need a guiding approach on how we want to implement this.
The main point that we will need a plan for is how to share data down the tree.
As of now, the Plot injects props into its children, which works, however it makes typing and discovery of what is passed a little trickier.

If we move the prop passing to a single shared context, then we can easily share the data, however, any change to the context value, will cause a re-render for any component (whether it needs the new data or not) that accesses it through the hook.

Re-rendering of child components was already a little bit of a problem as the props that were injected weren't properly memoized (see #1314)

The ideal approach would be one that only causes re-rendering to the components that actually require it.

I'm starting this thread so that we can start talking about approaches, since it seems that a lot of people are interested in helping with the migration to hooks.

[chrisirhc]

For folks interested in looking at the hooks conversion: how about putting up some RFCs/proposals on approaches for discussion? So we can weigh pros/cons?

[reitlepax]

f we move the prop passing to a single shared context, then we can easily share the data, however, any change to the context value, will cause a re-render for any component (whether it needs the new data or not) that accesses it through the hook.

You are right and this is not a trivial issue. If I recall believe the react-redux project was confronted to the same issues when migrating to hooks : how to have components subscribing to a context and make them re-render only when a subset of this context changes?

The approach I would suggest for the hooks migration would be to work in this order:

1. Transform all components using lifecycle methods to functional components with hooks while keeping the same API
2. Make a major release with react 16.8 as a peerDependency
3. Create a branch and work on @Xiot's suggestion to change the way we share data down the tree (the only component I found cloning its children and passing them props are XYPlot, Animation and AxisTicks)
4. If we succeed in keeping the same performance but without passing props directly to children from the plot (e.g with React context), deploy these changes in a minor release

My reasoning with this order is to have a modern (with hooks) version as soon as possible, which would allow us to work on several tasks in parallel from that, such as using a css-in-js library and working on the "sharing data down the tree" issue.

After some digging, I have found only a few places were some react lifecycle methods were not used in a recommended way (see here for what I mean by "recommended")

Using lifecycle to state to store in react state some data derived from props:

• ArcSeries
• Treemap

Using react state as a cache:

• XYPlot

Imo we shouldn' have much trouble to update ArcSeries and Treemap with hooks if we try to keep a behavior close to the current one (i.e by keeping the Plot passing data directlty to children)

As for XYPlot, from what I understand the caching @Xiot fixed in this PR does bring significant perf improvements, which is great and imo we should aim to have the same performance when migrating it to a functional component. This should be doable with hooks if we keep passing down props to children in a first iteration.

That means that we should be able to reach the step 1 "Transform all components using lifecycle methods to functional components with hooks while keeping the same API" rapidly and in the meantime we can explore several other options to share data down the tree (with react context for instance).

What do you think ?

[Xiot]

I really like your migration plan.
I have an idea for surgical updates, so I'll play with it and push up a PR.
The basic premise is that it will work somewhat like recoiljs (I haven't looked through all of their code yet, and I definately don't want to take a dependency on it)

The basic concept is that the the plots will have a context that exposes an event that the children can listen to. When the plot has updated data, then it will fire the event for the specific child. The child will then cause itself to re-render. I've done this in the past and it worked fairly well.

[Xiot]

Here is a sandbox with the basic concept working
https://codesandbox.io/s/runtime-fog-u58pz?file=/src/context.js

Basically it creates a context that should never really change.
Updates are handled by setting the value on a ref.
Consumers can subscribe to a specified key to get updates when that value changes.

Another variation of this approach would be to use something like selectors. I'll probably play with that now

[reitlepax]

The basic premise is that it will work somewhat like recoiljs
That's what I was thinking, and I agree that we shouldn't have it as a dependency but we might want to borrow the same concept.

The other option I had in mind was to create several context, and a child component can subscribe to only the ones it needs, e.g PlotContext, DataContext, AnimationContext, but it would be hard to maintain and we would end up with some components needing only asubset of DataContext and a subset of PlotContext, and they would re-render too often.

I like your approach because it can scale better imo, I will look into the sandbox to see how that works!

[chrisirhc]

The basic premise is that it will work somewhat like recoiljs
That's what I was thinking, and I agree that we shouldn't have it as a dependency but we might want to borrow the same concept.

The other option I had in mind was to create several context, and a child component can subscribe to only the ones it needs, e.g PlotContext, DataContext, AnimationContext, but it would be hard to maintain and we would end up with some components needing only asubset of DataContext and a subset of PlotContext, and they would re-render too often.

I like your approach because it can scale better imo, I will look into the sandbox to see how that works!

I actually really like this idea. It'd be easy to reason with why something re-rendered. I.e. a component re-rendered because animation context has changed.

It leverages the features of React without introducing another event bus.

Mentioned to @Xiot before (offline) that I'm a little concerned with reaching into adding a new concept/abstraction so soon.

One other concern I had is that we're not really measuring performance throughout these changes. Would it make sense to put some performance measurements in our current test suites? E.g. count number of re-renders?

[reitlepax]

One other concern I had is that we're not really measuring performance throughout these changes. Would it make sense to put some performance measurements in our current test suites? E.g. count number of re-renders?

I really like the idea of putting performance measurements before trying to improve them. The number of re-renders is a good metric imo since in our situation that is what we try to reduce when we talk about improving perfs

[Xiot]

UI performance is definatley tricky to capture as it can't really be captured accurately outside of the browser accurately.
I would suggest adding in window.performance to start collecting this in the current build.
Once we have some baseline measurements, we can pull in #1314 to see how it affects the metrics.

It'd be easy to reason with why something re-rendered. I.e. a component re-rendered because animation context has changed.
The issue with multiple contexts is that you potentially need different contexts per series, which doesn't really work with an unknown number of series.

If we don't want to introduce new abstractions, then keeping with data passing, and manually caching it, will probably be good enough for now.

We should probably start another issue for performance / optimization discussions.

[reitlepax]

Last time I missed out several components in react-vis packages using lifecycle methods.

This time, here is (I think) an exhaustive list of class components currently using lifecycle methods:

packages/react-vis:

• CanvasWrapper - need to rework how drawLayer is used
• Animation - rework how _updateInterpolator is used
• XYplot componentWillReceiveProps is used to cache props passed to children
• ArcSeries - state is only used to store derived data
• Treemap - state is only used to store derived data
• make-vis-flexible component factory - used to listen to window's resize only once for all the "flexible" components rendered on screen . A comment explains it is used as a "performance enhancement". I found a way to update it with hooks and keep the "listen once" mechanism as-is and I have tested with logs that the resize function is not called more often, but without a real perf benchmark I can't go further than that to prove it works

packages/showcase (easy to fix, and not as high priority imo since it doesn't cause bugs in the lib):

• ClockExample
• ResponsiveScatterPlot
• ForceDirectedGraph

packages/website (not a priority imo, but good to know):

• Hero

I have already started working on migrating several of them on my fork, @chrisirhc @Xiot do you want to use this issues to track the work on this component list, or shall I create another one ?

[Xiot]

Didn't we want to add performance markers first?
We may also want to do one at a time so that we can come up with a decent strategy of how best ot do these.

The bigger migration, imo, is getting rid of the inheritance and moving to functional components.

[reitlepax]

Didn't we want to add performance markers first?

Sure, sorry if I was unclear... the aim for my comment was only to list which components should be migrated and to ask you where we should track them

[reitlepax]

When I said I try to play with hooks migration on my fork it was just to indicate I was confident about which challenge we would face since I already tried to do the migration

[Xiot]

Ah...
I would create a new issue and list out all of the components (with checkboxes) that track the migration of the components. We can then update it (and reference it on the PRs) so that we have a single place to keep track.