Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert XYFrame from class to function component #606

Merged
merged 5 commits into from
Jan 20, 2022

Conversation

alexeyraspopov
Copy link
Member

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
Copy link

vercel bot commented Jan 19, 2022

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
Copy link
Member

emeeks commented Jan 19, 2022

@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
Copy link
Member Author

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

@emeeks emeeks merged commit f9552e5 into main Jan 20, 2022
@alexeyraspopov alexeyraspopov deleted the xyframeFunctionComponent branch January 20, 2022 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants