generateCategoricalChart - What will we do?

[ckifer]

Recharts uses a HUGE generator function called generateCategoricalChart to generate most charts. This function is a lot of things:

1. Way too many lines of code (2284 LOC!)
2. A class based component
3. Causes performance issues
4. Renders React elements not by the elements that are provided, but by checking for those elements in the React component tree and performing certain actions (cloneElement, etc.) if they are there
5. Prevents rendering of certain components if they are not valid (without letting the user know what is valid and what is not!!)

This file needs quite the lift in order to refactor and retain current functionality. How will we refactor? What has potential to break (everything?)? What do we know? What do we have no clue about? Which components do NOT rely on generateCategoricalChart?

This and more are things that we need to know in order to make important decisions about the future of this library.

Use this as a thread to discuss

Adding @nikolasrieble @akamfoad @Yilun-Sun
Some thoughts in this thread

[nikolasrieble]

Further observations:
It is a single entry point for most - but not all charts. It seems to me that we could split it up by layout: 'centric', layout: 'radial', and undefined layout

Charts that use generateCategoricalChart:

• PieChart
• AreaChart
• ComposedChart
• BarChart
• LineChart
• RadarChart
• ScatterChart
• FunnelChart
• RadialBarChart

Charts that do not:

• Sankey
• TreeMap

Thus we could:

• Create a variant generateRadialChart(), generateCentricChart(), and generateCartesianChart() and in each delete all the logic that is unused.
• Or we could start taking out one chart after the other, reimplementing it afresh without using generateCategoricalChart in the first place.

Either way forward, we should add tests.

[ckifer]

Looks like RadialBar is mentioned in both lists here - I think both Radar and RadialBar are generated with generateCategoricalChart

[ckifer]

It might be an interesting experiment to try to refactor something that does not use the generator function (Treemap or Sankey) and see what the experience there is like. We could use learnings from that to influence how we approach the rest. I personally like moving away from the generator entirely, but it remains to be seen how feasible that is, so I think the incremental approach could work well.

[nikolasrieble]

Thank you for the correction. Fixed it.

[nikolasrieble]

Which chart do you think would be the best candidate to take out?
Naively, I would say one of: RadialBarChart, FunnelChart, RadarChart, PieChart

But no matter where we start, we should set out rules for acceptance:

• What are the requirements for an new implementation?
  Functional implementation. No usage of ANY.

• Also, what about backwards compatibility?

(a) We could possibly just improve the behaviour right away, and release it in a major upgrade, no longer supporting the old version.
or
(b) The behaviour should be identical to the existing one. The API must be identical (or a superset).

[ckifer]

Yeah - I agree with your suggestions of places to start. Maybe one that isn't super complicated - Pie would be good.

Function components for sure

Certainly no use of any if we are going for a major version. If we aren't we'll have to be flexible.

I would say trying not to reach for helper functions like 'findAllByType'

Readable, utilities in a places that make sense. Small testable functions (but not overly so).

High test coverage.

I think initially we should do it in a non-breaking way if possible (at least in terms of API) because it will help us understand things better. Once we release one or two components with these changes we should shift mode and make all the breaking changes.

My concern is staleness of this branch/wherever this work might get done.

[PavelVanecek]

So I spent some time with the library and I'm thinking about few ways forward.

It's a bit of a wall of text but TL;DR is: push model (components push props to shared configuration) or pull model (components read shared state) or something else?

First some observation

A. Recharts has a truly unique API for custom components where instead of passing in React.Component, it accepts React.Element instances. Example: <Label content={<CustomizedLabel external={external} />} />
B. It pre-computes and then reuses tons of information in the CategoricalChart state.
C. Element clones, element clones everywhere. There are even two layers of cloning:
C1: first if element is defined with a content prop like in example (A)
C2: and then second time inside generateCategoricalChart.
D. The exported elements are essentially just markers. (Some less and some more: XAxis is purely a "displayName holder", but ReferenceLine has some rendering in it.) The real rendering happens inside generateCategoricalChart.

Now my opinion

• I was confused by (A) at first but going forward I don't really see a problem with it. It's easy to use definitely.
• I feel we should be trying to keep the pre-computing (B) and reuse as much as possible
• I am afraid that the element cloning based on displayName (C2) is brittle and is one React update away from a complete disaster. I would aim to get rid of that ASAP.
• I did not try to measure the performance of (C) element cloning. It might be faster or slower than actually re-rendering individual elements?
• Chart components do not render when wrapped in custom components #2788 is actually why I'm here - I was wondering what crazy stuff is going on inside the library that it doesn't allow to group components to subcomponents. Now I know.
• I don't think React is a good framework choice for charts but that train has left the station.

My thought process

1. First I was thinking we could refactor all elements to be small individual independent Components. But looking at (B) that doesn't seem possible - there will always be some shared, pre-computed state and these components are not going to be independent.

2. We cannot get rid of (C1) cloning while preserving backwards compatibility - a major release will be needed. We might prepare a migration script or something. So let's not worry about (C1) until later.

3. We can get rid of (C2) cloning while keeping API the same - the public and documented API, that is.

4. So let's focus on removing (C2) cloning first.

5. WRT to (D) markers we can either

5a. Embrace the "marker elements idea" completely:

• Turn this into a feature, but a better one.
• Stop relying on displayName.
• Instead of parsing children array and reading props, have each component push their existence (and their props) to a shared config.
• generateCategoricalChart will still have to render each component but it won't have to include dozens of if layout == 'horizontal' because that information would have been pushed by the components.
• This looks like a "Push model"
• Pros: thanks to the push we will know exactly what information to pre-compute and which computations we can skip
• Cons: generateCategoricalChart might need to stay fairly large. It might be possible to refactor further to separate rendering into yet another layer of components. Bonus: the push + compute + render might be even slower?

5b. Or forget the markers:

• Move all rendering inside the individual elements
• Reduce generateCategoricalChart to be "just" the root level component that pre-computes and caches state.
• Each component will then pull what it needs from the state.
• So I would think about this as a "Pull model"
• Pros: generateCategoricalChart will become quite slim because all the rendering will go away.
• Cons: it might be tricky knowing which data to pre-compute and cache; lazy computations might be too slow?

5c. Some completely different approach?

6. After that is done, we will have free hands to split generateCategoricalChart into smaller functions. It might turn out to be not necessary - or perhaps we will find that we can have NumericalChart and CategoricalChart and that will be enough for all?

If you made it this far, thank you for your attention. And what do you think? (5a) or (5b) or (5c)?

[nikolasrieble]

Thank you for diving into the problem!

I am afraid that the element cloning based on displayName (C2) is brittle and is one React update away from a complete disaster. I would aim to get rid of that ASAP.

I agree that we should focus on removing the reliance on the displayName. Lets us focus on that. What are the next steps here? Would you like to own this refactoring? @PavelVanecek

The choice between (5a) and (5b) should be prepared with more investigation. Performance is critical, and you mention it as a potential con in both options (i.e. slow). Lets evaluate performance and see which one results in better behaviour.

[PavelVanecek]

Yes so here's what I propose:

1. Set a performance test suite so that we know where we are and where we're going. Doesn't need to be integrated to github and doesn't have to be entirely automated but let's have something measurable. This test suite might already exist but I am not aware of it.
2. Choose two components and prepare a Proof of Concept branch for each (5a) and (5b). I would choose the most complicated ones so that we find roadblocks sooner rather than later.
3. Then we will see how each performs and feels and can come back to the decision better armed.

Sounds good? I can volunteer to start.

[nikolasrieble]

Sounds great! Could you please start by writing separate issues that we can then assign to you and link here? Thereby we can track work related of this github discussion.

[PavelVanecek]

Created:

• Create performance tests #3822
• Proof of Concept of plan 5a #3823
• Proof of Concept of plan 5b #3824

I also think we should work on #3437

[PavelVanecek]

Update!

After two PoCs:

• 5a: DO NOT MERGE: Request for comments: Plan 5a Proof of concept #3852
• and 5b: DO NOT MERGE: RFC: Plan 5b Proof of concept #3965

We have decided to go forward with 5b.