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

upgrade react to v18 and react pinch pan to v3 #3958

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

Conversation

farooqkz
Copy link
Collaborator

@farooqkz farooqkz commented Jun 19, 2024

Let me know if there is any problem.

Softly depends on removal of BluePrint as upgrading React to v18 also requires upgrading BP. And as BP will get removed, we'll have a wasted effort here.

Closes #3957

@farooqkz farooqkz requested review from Simon-Laux and WofWca June 19, 2024 07:58
@farooqkz
Copy link
Collaborator Author

I don't know how to fix the remaining typing errors about contextType

@@ -14,6 +14,7 @@
- use `SOURCE_DATE_EPOCH` environment var for build timestamp instead of `Date.now()` if set.
- use italic variants of Roboto font correctly #3949
- show chat name when searching in chat #3950
- upgrades react to v18 and react pinch pan zoom to v3
Copy link
Member

Choose a reason for hiding this comment

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

move that to the top, your change is not released yet ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sowwy

@Simon-Laux
Copy link
Member

This needs to be tested carefully before merging because it is a big change that might introduce new bugs.

useful links:

typescript now needs explicit children in the props everywhere https://react.dev/blog/2022/03/08/react-18-upgrade-guide#updates-to-typescript-definitions

@Simon-Laux
Copy link
Member

pr looks good code wise so far 👍

@Simon-Laux Simon-Laux marked this pull request as draft July 4, 2024 22:29
@nicodh
Copy link
Contributor

nicodh commented Jul 18, 2024

Maybe test and review again after #4006

@WofWca
Copy link
Collaborator

WofWca commented Sep 18, 2024

I rebased and fixed the "context" errors.

Things that don't work:

  • composer's emoji picker
  • Opening a profile modal prints an error

    Warning: findDOMNode is deprecated and will be removed in the next major release.

I think this is because of

 WARN  Issues with peer dependencies found
packages/frontend
└─┬ @blueprintjs/core 4.20.2
  └─┬ react-popper 1.3.11
    └── ✕ unmet peer react@"0.14.x || ^15.0.0 || ^16.0.0 || ^17.0.0": found 18.3.1

So yeah, either a Blueprint upgrade or Blueprint removal (#4006) is needed.

Otherwise: I checked the React 18 migration guide and I didn't notice anything that we need to do, though I didn't really understand some things from the guide.

@WofWca WofWca force-pushed the far-react-upgrade branch from 492c0d4 to 26a4dc8 Compare October 15, 2024 13:51
@WofWca
Copy link
Collaborator

WofWca commented Oct 15, 2024

I rebased again, and fixed the emoji picker not opening issue: 26a4dc8. And I suspect we might have a few more such problematic places where timing matters. I think this has to do with this item from the React upgrade guide:

Consistent useEffect timing: React now always synchronously flushes effect functions if the update was triggered during a discrete user input event such as a click or a keydown event. Previously, the behavior wasn’t always predictable or consistent.

Another issue that I don't think I've seen happen outside of this MR:

reply.scroll.mp4

I believe it has to do with this recently added code:

const observer = new ResizeObserver(entries => {
const entry = entries[0]
if (!entry) {
return
}
const newHeight = el.clientHeight
if (newHeight === prevHeight) {
// In case only width changed or something.
return
}
const scrollDistanceToBottomBeforeResize =
el.scrollHeight - el.scrollTop - prevHeight
if (
scrollDistanceToBottomBeforeResize <=
maxScrollToBottomDistanceConsideredShort
) {
el.scrollTop = Number.MAX_SAFE_INTEGER
// Sometimes this spews out negative numbers when we're scrolled
// all the way to the bottom and then resize the window.
// We'd expect this to be 0 in that case, but as long as
// it works it's fine I guess.
log.debug(
`Message list resized, and distance to bottom was` +
` ${scrollDistanceToBottomBeforeResize} before the resize.` +
` Scrolling to bottom.`
)
}
prevHeight = newHeight
})
observer.observe(el)
return () => {
observer.unobserve(el)
}
}, [])

Other surface-level things appear to work fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to React 18
4 participants