DO NOT MERGE: RFC: Plan 5b Proof of concept

[PavelVanecek]

Compare to #3852

I myself prefer this approach - I think there's less indirection this way. But I am afraid that some components need props from other components and we end up with a mix anyway. Let's see.

This branch will not pass tests and it doesn't actually render the tooltip in storybooks but it's already 11pm.

I want to show the idea so that we can decide which way forward to go.

Description

Instead of cloning elements and resetting props, generateCategoricalChart will send the "internal" props through context.

Tooltip reads both props (coming from users) and context (computed internally) and renders.

Related Issue

#3717

#3852

Motivation and Context

Solves:

1. Cloning
2. Mixed internal and external props

How Has This Been Tested?

I tested, doesn't work, don't merge. Only for discussion

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• I have added a storybook story or extended an existing story to show my changes
• All new and existing tests passed.

[ckifer]

Nice, good work

I also prefer this approach - I'd like to see it in action but overall no qualms with the direction

[nikolasrieble]

I agree, this approach is much cleaner. I wonder though about performance. One large context would lead to many re-renders, would it not?

[ckifer]

One large context would lead to many re-renders, would it not?

The real question is does it end up being more re-renders than currently happen? If not then 🚀. If it makes re-renders go wild then yeah we may need separation somehow.

Unfortunately everything is so tightly coupled so thats probably easier said than done

[PavelVanecek]

So conceptually we should be the same:

Before: one large state, re-render on every change
After: one large context, re-render on every change

I hope that this will unlock more opportunities for optimization, like pure components or splitting contexts. But we should be able to get to the same performance with fairly low effort IMHO.

[lunelson]

I'm late to this discussion, but was reading the other thread about possible approaches to break down the monolithic generator function and also wanted to suggest the context approach, essentially.

This explainer on the react-aria-components site is a nice overview of how their architecture does this at every level, through a combination of contexts and optional hooks.

Re: the question of "re-rendering on every change", two things: 1. use multiple contexts, one for the common props, and one for each of the chart-type-specific props; and 2. consider using somethin like zustand vanilla stores via context, which allow granular updates

[ckifer]

#4193

it looks like this approach breaks the "panorama" feature because context always stores the height/width, etc. of the outer chart. When someone places a chart inside Brush, the elements that have been migrated to use context continue to reference the outer chart state and render in the sub-svg in the incorrect position. We'll need to find a way around that (causes issues in 2.11+)

Edit nvm all good, just need another separate provider returned