Refactor Antd Table columns #7772

hotzenklotz · 2024-04-26T13:49:39Z

In preparation for upgrading to React 17 (#7765), I ran into trouble with Typescript and infinite recursions ("type definition to deep") resulting in crashes. Most likely this is not caused by the React update per se but some other dependency, likely any from the rc-* packages used by antd.

In particular, Typescript was unhappy with the type RowRenderer in any of the table <Column> elements:

<Column
   ...
  render: (_name: string, rowRenderer: RowRenderer) => rowRenderer.renderNameColumn()
/>

Instead of using <Column /> children for the FixedExpandableTable, I refactored this components to use objects and props instead, e.g. <Table columns={ ... } />. Judging from the antd documentation this seems to be the preferred way these days. (Similar to <Dropdown />, <Menu /> etc).

Steps to test:

Make sure that Dashboard > Datasets table works
Make sure that Admin > Tasks table works

Issues:

Contributes to Update React to v17 #7765

(Please delete unneeded items, merge only when none are left open)

Updated changelog
Updated migration guide if applicable
Updated documentation if applicable
Adapted wk-libs python client if relevant API parts change
Removed dev-only changes like prints and application.conf edits
Considered common edge cases
Needs datastore update after deployment

This reverts commit 1cf2f45.

MichaelBuessemeyer

Thanks for fixing this 🎉

Judging from the antd documentation this seems to be the preferred way these days. (Similar to ,
etc).

Yes, you are correct as far as I remember 👍

Thanks for this PR. Please find my comments below.

One thing: The PR does not state why moving the column definition into an object instead of an react child fixes the Typescript hiccup. In case you know why, please add a short explanation in a comment in case someone later comes back to this PR and thus gets the proper reasoning / understanding for this change 🙏

Once the mini feedback is applied I'd say its all green 🟢

frontend/javascripts/components/fixed_expandable_table.tsx

MichaelBuessemeyer · 2024-04-30T08:50:08Z

CHANGELOG.unreleased.md

@@ -26,6 +26,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
 - Toasts are shown until WEBKNOSSOS is running in the active browser tab again. Also, the content of most toasts that show errors or warnings is printed to the browser's console. [#7741](https://github.com/scalableminds/webknossos/pull/7741)
 - Improved UI speed when editing the description of an annotation. [#7769](https://github.com/scalableminds/webknossos/pull/7769)
 - Updated dataset animations to use the new meshing API. Animitation now support ad-hoc meshes and mappings. [#7692](https://github.com/scalableminds/webknossos/pull/7692) 
+- Slightly refactored the `<FixExpandleTable/>`component to use columns as props. [#7772](https://github.com/scalableminds/webknossos/pull/7772)


I'd say that the changelog entry is not necessary as these changes are not user facing. But feel free to keep this :)

MichaelBuessemeyer · 2024-04-30T09:00:20Z

frontend/javascripts/dashboard/advanced_dataset/dataset_table.tsx

+        key: "name",
+        sorter: Utils.localeCompareBy<RowRenderer>((rowRenderer) => rowRenderer.data.name),
+        sortOrder: sortedInfo.columnKey === "name" ? sortedInfo.order : undefined,
+        render: (_name: string, rowRenderer: RowRenderer) => rowRenderer.renderNameColumn(),


You still have the RowRenderer type here. So moving this type into the column prop definition fixes the bug / recursive definition? Sorry, the PR description wasn't that clear to me to why this is actually fixing the typescript hiccup.

Co-authored-by: MichaelBuessemeyer <[email protected]>

hotzenklotz · 2024-05-06T08:26:38Z

One thing: The PR does not state why moving the column definition into an object instead of an react child fixes the Typescript hiccup. In case you know why, please add a short explanation in a comment in case someone later comes back to this PR and thus gets the proper reasoning / understanding for this change 🙏

I don't know why React v17 + Typescript ran into an issue. I can only assume that TS tries to automagically infer types for <Table /> children and that the React v17 internals changed somehow which leads to an infinite recursion and TS crashing. The solution in the PR applies more(manual) type hints for the column children leading to less dark type magic by TS.

MichaelBuessemeyer

Thanks for applying the suggestion.

Let's 🚢

MichaelBuessemeyer

I think I found a bug, let me quick investigate before merging

MichaelBuessemeyer

I found the bug: It was cased by name shadowing. Please see my comments below.

After this is fix, please ask for a quick re-testing that then it should be ready to merge (hopefully 🙈).

And sorry for the take backsies, but it was necessary

frontend/javascripts/admin/task/task_list_view.tsx

MichaelBuessemeyer · 2024-05-06T11:44:28Z

frontend/javascripts/admin/task/task_list_view.tsx

+          </div>
+          {task.status.pending > 0 ? (
+            <div>
+              <LinkButton onClick={_.partial(assignTaskToUser, task)}>


Here is an example that causes the rendering to crash as the current code shadows the lodash variable

MichaelBuessemeyer · 2024-05-06T11:44:42Z

frontend/javascripts/admin/task/task_list_view.tsx

+            </div>
+          ) : null}
+          <div>
+            <LinkButton onClick={_.partial(deleteTask, task)}>


Here as well btw :)

MichaelBuessemeyer · 2024-05-06T11:47:09Z

frontend/javascripts/admin/task/task_list_view.tsx

-            key="actions"
-            width={170}
-            fixed="right"
-            render={(__, task: APITask) => (


The previous code seems to have used two underscores to avoid the shadowing. Imo using _unused is more explicit and easier to spot than using two underscores to avoid the name shadowing. Therefore, the _unused approach is more explicit and therefore imo more preferable.

Co-authored-by: MichaelBuessemeyer <[email protected]>

MichaelBuessemeyer

Now it works 🎉

…le-columns

…nto table-columns

hotzenklotz added 5 commits April 26, 2024 15:31

refactor antd table columns as arrays instead of react elements

4cca40e

fix linting

6ed0ac1

fix dayjs types

21876cd

formatting

88bc730

Revert "fix dayjs types"

8f35114

This reverts commit 1cf2f45.

hotzenklotz self-assigned this Apr 26, 2024

hotzenklotz requested a review from MichaelBuessemeyer April 26, 2024 13:49

hotzenklotz added the frontend label Apr 26, 2024

updated readme

fc1eefb

hotzenklotz mentioned this pull request Apr 26, 2024

Update React to v17 #7765

Merged

11 tasks

MichaelBuessemeyer requested changes Apr 30, 2024

View reviewed changes

Update frontend/javascripts/components/fixed_expandable_table.tsx

b0ca82f

Co-authored-by: MichaelBuessemeyer <[email protected]>

MichaelBuessemeyer approved these changes May 6, 2024

View reviewed changes

MichaelBuessemeyer self-requested a review May 6, 2024 11:34

MichaelBuessemeyer requested changes May 6, 2024

View reviewed changes

MichaelBuessemeyer self-requested a review May 6, 2024 11:42

MichaelBuessemeyer requested changes May 6, 2024

View reviewed changes

Update frontend/javascripts/admin/task/task_list_view.tsx

129fa85

Co-authored-by: MichaelBuessemeyer <[email protected]>

MichaelBuessemeyer self-requested a review May 6, 2024 14:25

MichaelBuessemeyer approved these changes May 6, 2024

View reviewed changes

Merge branch 'master' into table-columns

94cdaf7

hotzenklotz enabled auto-merge (squash) May 13, 2024 06:56

hotzenklotz added 4 commits May 13, 2024 09:23

Merge branch 'master' into table-columns

9342ae4

fix typing error

b368191

Merge branch 'master' of github.com:scalableminds/webknossos into tab…

ece7de7

…le-columns

Merge branch 'table-columns' of github.com:scalableminds/webknossos i…

98b535a

…nto table-columns

hotzenklotz merged commit d08a865 into master May 14, 2024
2 checks passed

hotzenklotz deleted the table-columns branch May 14, 2024 07:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Antd Table columns #7772

Refactor Antd Table columns #7772

hotzenklotz commented Apr 26, 2024

MichaelBuessemeyer left a comment •

edited

Loading

MichaelBuessemeyer Apr 30, 2024

MichaelBuessemeyer Apr 30, 2024 •

edited

Loading

hotzenklotz commented May 6, 2024

MichaelBuessemeyer left a comment

MichaelBuessemeyer left a comment

MichaelBuessemeyer left a comment

MichaelBuessemeyer May 6, 2024

MichaelBuessemeyer May 6, 2024

MichaelBuessemeyer May 6, 2024

MichaelBuessemeyer left a comment

Refactor Antd Table columns #7772

Refactor Antd Table columns #7772

Conversation

hotzenklotz commented Apr 26, 2024

Steps to test:

Issues:

MichaelBuessemeyer left a comment • edited Loading

Choose a reason for hiding this comment

MichaelBuessemeyer Apr 30, 2024

Choose a reason for hiding this comment

MichaelBuessemeyer Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

hotzenklotz commented May 6, 2024

MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

MichaelBuessemeyer May 6, 2024

Choose a reason for hiding this comment

MichaelBuessemeyer May 6, 2024

Choose a reason for hiding this comment

MichaelBuessemeyer May 6, 2024

Choose a reason for hiding this comment

MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

MichaelBuessemeyer left a comment •

edited

Loading

MichaelBuessemeyer Apr 30, 2024 •

edited

Loading