fix(DataTable): sort table on initial render with `initialSortColumn` or `initialSortDirection` props

[bwittenberg]

Closes #3951

Changelog

New

Changed

• The useTable hook is responsible to sort the data for the DataTable. If the data changes, then it sorts according to the current sort state. It should also sort data on initial render. This PR updates the initial state of useTable so that the data is sorted on initial render.
• DRY up the test by extracting functions that read headers and rows.
• Add Jest tests

Removed

Rollout strategy

Answering this question led to a questions about the purpose/intended use of this component. In my experience with data tables, consumers either have all the data on the client and want to render it in the table, or consumers do not have all the data, and may need to fetch more.

If consumers have all the data, then it's possible to sort and filter on the client, and then render correct results. I'll name this "completeData" mode. I wouldn't use this name as part of the API, but it works for this discussion.

If consumers do not have all the data, then it's not possible to sort and filter on the client, because all client side sorts and filters operate on a partial data set. For correct results, the sort and filter must be done on the server, where the complete data set is known. I'll name this "partialData" mode.

Why discuss these two different usages in this PR?

Prior to this change, on initial render only, the data table would not sort the passed in data. For consumers with partial data, this is the desired behavior. After this change, consumers that are using this component with partial data will notice that the data is now sorted on the client on initial render.

Based on the current DataTable API, I think this component was designed for "completeData" mode, because any sorting and renders after the initial render only sort the data that's in the component state.

To support "partialData" mode, this component would need to allow consumers to pass in the "sortColumn" and "sortDirection", and also expose callbacks so consumers can respond to sort change like onSortChange.

To handle the "partialData" and "completeData" modes of a table, it could make sense to build a "controlled" version of the DataTable, maybe something like StatelessDataTable, and then the DataTable can remain the "uncontrolled" version that adds state to the StatelessDataTable.

After looking at more of the components in DataTable, for the "partialData" mode, consumers could use Table directly with import { Table } from '@primer/react/experimental'.

Apologies for the lengthy discussion. I suspect that the authors already know all of this, so this is mostly me trying to convince myself that this change should be considered a patch, because the DataTable is designed for the "completeData" mode, and consumers are probably not expecting the buggy behavior.

• [x] Patch release
• [ ] Minor release
• [ ] Major release; if selected, include a written rollout or migration plan
• [ ] None; if selected, include a brief description as to why

Testing & Reviewing

• Visit http://<storybookurl>/?path=/story/drafts-components-datatable-features--with-custom-sorting, assert data is sorted by updated at, click the updated at column, assert data is sorted in opposite direction, click Repository header, assert data is sorted by Repository, click Repository again, assert sort changes direction

Merge checklist

• [x] Added/updated tests
• [ ] Added/updated documentation
• [ ] Added/updated previews (Storybook)
• [ ] Changes are SSR compatible
• [x] Tested in Chrome (version 124.0.6367.207)
• [x] Tested in Firefox (version 126.0)
• [x] Tested in Safari (version 17.4.1)
• [x] Tested in Edge (version 124.0.2478.109)
• [ ] (GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: 3370857fe47151d916274dd53a6577a3db1995bd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[TylerJDev]

Hello @bwittenberg, thank you for the PR! Also thank you for providing detailed comments on what this PR aims to achieve.

The one potential issue that could arise from this change, is that that the state would render twice, as the conditional if (data !== prevData) would always equal true upon initial render as data is null, causing a re-render with state being updated within that conditional. This seems fine, but I wonder if we could sort the data initially when we initialize rowOrder, (e.g. sorting the data within useState). This way, the data will always be sorted on initial render, and we don’t need to worry about a double re-render. What do you think about this approach?

For example:

const [rowOrder, setRowOrder] = useState((data) => {
 // sort the data when we store it in state
})

[bwittenberg]

@TylerJDev , thanks for the review! I'm happy to implement your suggestion, but wanted to provide some thoughts for you to consider. Please let me know which option you prefer, or maybe you have another option in mind that I'm not seeing. If you prefer changes to the original PR, I can run through the PR checklist again.

FWIW, I prefer Option3, but again, happy to follow your guidance.

Option1: Avoid the call to setState on initial render (like you suggested)

I implemented this suggestion here: https://github.com/bwittenberg/primer-react/pull/1/files

Pros: Improves initial render performance because the hook will only run once. Cons: More code changes, so there's a higher risk of bugs.

Option2: Keep call to setState on initial render (no changes to PR)

Pros: Least amount of code changes. Cons: Less performant than Option1

Although this is less performant than Option1, it's unclear to me how much less performant it will be. This should have similar performance characteristics to updates when the data prop changes.I did some testing with breakpoints and discovered that while React will not "render" the children twice, it will call the DataTable component function twice, which iterates over the headers and rows.

The React docs discuss the technique of calling set state during render in the "Storing information from previous render" section in the docs, and React has an optimization to avoid an extra render.

This pattern can be hard to understand and is usually best avoided. However, it’s better than updating state in an effect. When you call the set function during render, React will re-render that component immediately after your component exits with a return statement, and before rendering the children. This way, children don’t need to render twice. The rest of your component function will still execute (and the result will be thrown away). If your condition is below all the Hook calls, you may add an early return; to restart rendering earlier.

I added a performance test to confirm the render count. I'd probably keep the test because it adds a little bit of value, and seems easy to maintain.

Here's an example of an early return that would improve performance on initial render and when certain props change, but I wouldn't advocate for this change, because I suspect it's a little known optimization technique and feels brittle: https://github.com/bwittenberg/primer-react/pull/3/files

Option3: Remove the redundant state

Example PR: https://github.com/bwittenberg/primer-react/pull/2/files

Pros: Improves performance for initial render and when props change. Removes redundant state and improves readibility. Cons: More code changes, so there's a higher risk of introducing bugs.

This option removes the rowOrder and prevData states, because they are not necessary for this hook to return sorted rows. Removing unnecessary state is recommended in the React docs

If the value you need can be computed entirely from the current props or other state, remove that redundant state altogether. If you’re worried about recomputing too often, the useMemo Hook can help.

[TylerJDev]

@bwittenberg, thanks so much for the response, and sorry about the delay on my part! I'll take a look at your proposals before the week is up!

[bwittenberg]

Sounds good @TylerJDev !

[TylerJDev]

I apologize for the long wait @bwittenberg, I completely lost track of time! 🙇

I'd say let's go with option 1, I think that will be the safer option, and will allow us to monitor for any potential regressions a bit easier since we don't need to worry about performance like we might for option 2.

Also I love the additional option 3! I'm not too familiar with the component to understand the full tradeoffs we might have with this option, so I'll cc @joshblack in case he wants to provide input here :grin:

Next steps would be:

1. Implement option 1
2. (Item for @TylerJDev) Do regression testing for active implementations for DataTable
3. Review and merge if there aren't any regressions or comments by anyone else on the team

[bwittenberg]

Thanks for the response @TylerJDev ! This is on my radar to fix!

[github-actions[bot]]

Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days.

[bwittenberg]

Hello @TylerJDev! Sorry for the long lapse in my communication. I decided to use some time for open source earlier this year, and then I started a new job right before I heard back here. I should have closed the PR then, because work really picked up quickly.

I thought I might re-open it now, however, after re-reading the discussion, I don't plan to continue the work on this. Although it's an easy fix, consumers may rely on the current behavior, so I think the more complicated part of this work is considering if and how a fix should roll out.

Hopefully this work has provided some value if anyone decides to pick up the original bug.