nteract / semiotic

A data visualization framework combining React & D3
https://semioticv1.nteract.io/
Other
2.43k stars 133 forks source link

Convert XYFrame from class to function component #606

Closed alexeyraspopov closed 2 years ago

alexeyraspopov commented 2 years ago

Alright, this one might seem messy, gonna try to describe all the details I've encountered.

The main reason to do this right now was the idea to make top level frames responsive by default (see #601). It would be a neat trick if it was possible by just changing the behavior inside <Frame />, but what makes it not that easy is that <XYFrame /> and others also need size as a part of their rendering phase. Thus, the proper place to modify to responsive-by-default behavior are those actual frames. Recently introduced hook for element size is great, but can't be used by class components. I have zero motivation to implement something new for class components, so the only way to go is to convert the rest of classes to functions.

There are a bunch of aspects I noticed that are similar to all top level frames that are currently implemented as classes:

  1. They use getDerivedStateFromProps to change the state based on changed props and none of them actually have state (that is controlled by setState)
  2. Initial state is computed in constructor and additional mechanic is implemented in getDerivedStateFromProps to avoid any possibility to rewrite existing state if underlying data from props haven't changed.
  3. There are a bunch of class methods that don't have too much dependencies on the rest of the class, so they can be treated as "functions" (and possibly become them)
  4. onUnmount prop. I'm seeing that some frames have this prop and I'm not sure about how it is supposed to be used (or should it be used by the users at all). in any case, I'd consider getting rid of it as it's domain itself is defined based on the notion and assumptions of class components. Basically, it feels like a code smell. cc @emeeks

Technically it wasn't hard to convert <XYFrame /> to function component. But there are still plenty of things to change inside this new component to make it benefiting more from being a function. These can be addressed in the following PRs:

  1. Separate state computation by state domains, don't keep everything in a single function that expects the whole set of props and state
  2. (Enabled by the previous item) get rid of custom state/props comparison logic. We can now rely on basic hook memoization
  3. Eliminate onUnmount prop
vercel[bot] commented 2 years ago

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nteract/semiotic/E5aURiVSfri3H15XccMuHi8b9kcQ
✅ Preview: https://semiotic-git-xyframefunctioncomponent-nteract.vercel.app

emeeks commented 2 years ago

@alexeyraspopov We should get rid of the onUnmount prop it was added for a very specific use case and we shouldn't support that anymore since you're right, it's ugly. I agree with #1 & #2 as well--exciting to see this come together so well.

alexeyraspopov commented 2 years ago

The only thing that relies on onUnmount prop is FacetController to track how the space is changing in order to recompute shared extents. I think it can be replaced with some React Context stuff. Gonna try to work around it