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

feat: Add Stable/useStable and use for GridTableProps.rows/columns #933

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

stephenh
Copy link
Contributor

@stephenh stephenh commented Aug 20, 2023

This PR attempts to solve the "damn it, columns was not memo'd so we blew up the table's performance and/or messed up the DOM focus" bugs by forcing columns and rows to really be stable.

  • Marks GridTableProps.rows and columns with the Stable<...> marker/branded type
  • This branded type can only be created by useMemo or useStable
    • We've extended useMemo's return type to be Stable<T> so existing useMemo-d values will be considered stable
    • useStable is a new hook that is "just useMemo" but requires its own dependency array to be stable, which is safer than useMemo which to date just has a handful of eslint heuristics to check invalid deps
  • Adds a GridTableUnsafe that is "the old non-stable API" to ease the transition
    • Granted, an easier migration path would be to keep GridTable as-is, and introduce a new GridTableSafe, and move pages over to it one-by-one ... tempted to just pull the bandaid with the breaking change, but it will be a large roll out effort
    • ...still have work to do in this PR to clean up ^ fallout...

@@ -18,7 +18,7 @@ import {
Filters,
GridColumn,
GridDataRow,
GridTable,
GridTableUnsafe as GridTable,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bdow I'd added GridTableUnsafe as an alias with the old/as-is props, but kinda wondering if maybe we should take this opportunity to introduce Table as the new component name, which has the new/safe props, and GridTable is the old/deprecated component name.

That would make this roll out a lot easier, b/c all exists clients of GridTable wouldn't have to be either a) renamed over to the GridTableUnsafe alias, or b) have their non-memoed rows/columns fixed.

...although arguably b) would be a great forcing function, it just means rolling this out will be a lot more work/updates.

Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I like that idea to rename the "new" to just Table. Then we won't block updating Beam on updating existing GridTable implementations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Pushed up the "GridTable stays the same, Table is the new safe one".

Copy link
Contributor

@bdow bdow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Would love to do a FE Guild discussion on this.

// With this extension, we could almost get away without our own `useStable` hook,
// but we can't use the extension mechanism to restrict useMemo's `deps` to be
// `ReadonlyArray<StableDep>`.
function useMemo<T>(factory: () => T, deps: ReadonlyArray<unknown> | undefined): Stable<T>;
Copy link
Contributor Author

@stephenh stephenh Aug 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bdow FWIW during demos, answering a question for @khalidwilliams , I'm kinda thinking we should back away from this change, which "upgrades" useMemo to produce stable values, and have useStable be the only one that can make stable values.

The reason is that, with the TS typing, I'm unable to "upgrade" useMemo's dependency list to be only stable values, and it is precisely this restriction that we'd need to have prevented the bug I fixed.

I.e. the bug I fixed, the const columns = useMemo(...) was "already useMemo-d" but the elevations dependency was itself an unstable array:

https://github.com/homebound-team/internal-frontend/pull/4159/files?w=1#diff-eb71015b91b3c704dd3488a15c973047da7e637251f2d4a5b2f467c0861c73f9R146

So, really, the fix we need to be 100% bulletproof to performance is not just: a) the user uses useStable or useMemo for the rows/columns props, but also b) any dependencies the user used for useStable or useMemo must themselves be stable.

And I cannot "fix" useMemo to have this restriction (it would also be a huge breaking change), so I think we'd have to just say "now all const rows = useMemo(...)s have to be const rows = useStable(...)s" and deal with the fallout.

I'm pretty convinced this is the right direction, even though it's a more disruptive change, b/c all of our useMemos lines need to become useStables--and also make sure their dependencies are stables, but that's the whole point, we need to have the stableness check be transitively applied to dependencies of our dependency.

Wdyt?

@stephenh stephenh marked this pull request as draft October 14, 2024 20:14
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