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

Yuhsuan/react 18 upgrade #2426

Merged
merged 16 commits into from
Dec 9, 2024
Merged

Yuhsuan/react 18 upgrade #2426

merged 16 commits into from
Dec 9, 2024

Conversation

YuHsuan-Hwang
Copy link
Collaborator

@YuHsuan-Hwang YuHsuan-Hwang commented Oct 25, 2024

Description

Patches golden-layout and upgrades to react 18.

Also

  • migrates the dialog workaround to react 18
  • migrates blueprint toaster to react 18 (ref)
  • upgrades react type definition
  • removes react-color type def (somehow does not work with react 18 type def)
  • patches react-split-pane type def (ref)
  • upgrades react-konva for react 18 compatibility
  • adds optional children to react component props type (ref)
  • fixes key related warnings
  • upgrades konva and react testing library, which were blocked by react 18 upgrade

Note that there is a warning from react-resize-detector. Investigated if it can be fixed by upgrading the package to v11, but found that the child function pattern <ReactResizeDetector/> is removed from v10. Only the useResizeDetector hook is supported, which can only applied with functional components. This can be solved by creating a wrapper component, but requires additional changes because it adds an extra div element which breaks the current structure. I suggest we work on the upgrade in a separate PR (perhaps the next dev cycle) and focus on golden-layout patching and react 18 upgrade in this branch.

Update: we could consider replacing react-resize-detector with blueprint <ResizeSensor/> + throttling.

Let's close #1974 after this PR is merged.

Checklist

For linked issues (if there are):

  • assignee and label added
  • GitHub Project estimate added

For the pull request:

  • reviewers and assignee added
  • GitHub Project estimate added
  • changelog updated / no changelog update needed
  • unit test added (for functions with no dependenies)
  • API documentation added (for public variables and methods in stores)

For dependencies:

  • e2e test passing / corresponding fix added / new e2e test created
  • protobuf version bumped / no protobuf version bumped needed
  • protobuf updated to the latest dev commit / no protobuf update needed
  • corresponding ICD test fix added (BackendService changed) / no ICD test fix needed (BackendService unchanged)
  • user manual prepared (for large new features)

@YuHsuan-Hwang
Copy link
Collaborator Author

YuHsuan-Hwang commented Nov 4, 2024

Updates:

  • Upgraded konva and react testing library, which were blocked by react 18 upgrade.
  • Investigated if the warning from react-resize-detector can be fixed by upgrading the package to v11, but found that the child function pattern <ReactResizeDetector/> is removed from v10. Only the useResizeDetector hook is supported, which can only applied with functional components. This can be solved by creating a wrapper component, but requires additional changes because it adds an extra div element which breaks the current structure. I suggest we work on the upgrade in a separate PR (perhaps the next dev cycle) and focus on golden-layout patching and react 18 upgrade in this branch.

Update: we could consider replacing react-resize-detector with blueprint <ResizeSensor/> + throttling.

@YuHsuan-Hwang YuHsuan-Hwang marked this pull request as ready for review November 4, 2024 07:21
@YuHsuan-Hwang YuHsuan-Hwang added awaiting code review For pull requests that require code review awaiting testing For pull requests that require testing labels Nov 4, 2024
@veggiesaurus
Copy link
Collaborator

nice work @YuHsuan-Hwang 😎 🚀

@YuHsuan-Hwang
Copy link
Collaborator Author

Note: Let's close #1974 after this PR is merged.

Copy link
Collaborator

@loveluthien loveluthien left a comment

Choose a reason for hiding this comment

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

It looks good on Mac and Ubuntu.

@YuHsuan-Hwang
Copy link
Collaborator Author

Filed #2433 for the react-resize-detector warnings.

@YuHsuan-Hwang
Copy link
Collaborator Author

Update: fixed the issue of auto scrolling not being triggered (found by e2e tests)

@@ -353,7 +353,7 @@ export class FilterableTableComponent extends React.Component<FilterableTableCom
return (
<Table2
className={className}
ref={table.updateTableRef ? ref => table.updateTableRef(ref) : null}
ref={table.updateTableRef ?? null}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From React documentation:

React will also call your ref callback whenever you pass a different ref callback. In the above example, (node) => { ... } is a different function on every render. When your component re-renders, the previous function will be called with null as the argument, and the next function will be called with the DOM node.

Therefore, FilterableTableComponent updates the ref whenever the table re-renders because there is an additional arrow function in the ref callback.

Moreover, the ref is set to null and then to the DOM node in the process. Before react 18, we use the ref after the two calls are complete. With react 18, the re-render timing somehow changes and we use the ref between the two calls —— the ref is (temporarily) null and the auto scroll is not triggered.

I remove the additional arrow function to avoid unnecessary reset of the ref, avoiding the entire issue.

Copy link
Collaborator

@kswang1029 kswang1029 left a comment

Choose a reason for hiding this comment

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

Overall this looks fine after some manual testing of konva related things. The e2e tests have some failures due to tooltips blocking element to be clicked. The FOV of multi-color blending on load changes due to performance changes while running e2e tests. These issues are addressed in an e2e branch.

@YuHsuan-Hwang YuHsuan-Hwang added awaiting merge For pull requests ready for merge or pending backend/protobuf changes and removed awaiting testing For pull requests that require testing awaiting code review For pull requests that require code review labels Dec 6, 2024
@loveluthien loveluthien merged commit a720692 into dev Dec 9, 2024
6 checks passed
@loveluthien loveluthien deleted the yuhsuan/react_18_upgrade branch December 9, 2024 06:46
@YuHsuan-Hwang YuHsuan-Hwang removed the awaiting merge For pull requests ready for merge or pending backend/protobuf changes label Dec 9, 2024
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.

Package updates for v4
5 participants