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 R3F to v8 #1119

Merged
merged 3 commits into from
Sep 12, 2023
Merged

Upgrade React to v18 and R3F to v8 #1119

merged 3 commits into from
Sep 12, 2023

Conversation

axelboc
Copy link
Contributor

@axelboc axelboc commented May 13, 2022

The big change with React 18, obviously, is the createRoot API that replaces ReactDOM.render. In terms of code, this affects the demo but most importantly the Html component (I think the new API makes the code clearer, actually). In terms of performance, this means we can now opt-in to concurrent rendering features (see below). That being said, there should already be some performance improvements under the hood, especially since R3F already makes use of such features internally.

ALL FIXED There's a bunch of peer dependency warnings that should go away little by little, as usual, notably with Storybook, react-slider, react-reflex, etc. Talking about react-reflex, this library is affected by the breaking change in @types/react that consists in the removal of the implicit children prop from component types, like Component. So for instance, const ReflexContainer: React.Component<Props, any> is supposed to now be written as const ReflexContainer: React.Component<PropsWithChildren<Props>, any>. I've added @ts-expect-error comments for now. This is now fixed.

In R3F v8, the breaking change that affects our codebase is the sourceEvent key in event objects being renamed to nativeEvent.

Obviously there's a lot more to these major releases that we can benefit from or should watch out for (I will put this list in a discussion thread once merged):

  • useTransition to improve the way we use Suspense (e.g. by not hiding the current slice while the next one is loading)
  • useDeferredValue to improve performance on a case-by-case basis by deferring heavy, low-priority state updates
  • changes to colour management in Three v0.139 (we're currently blocked at v0.135 due to a breaking change Update three.js to r141 #1170), thanks to a change in Three 0,139, we no longer have to set the linear prop on R3F's Canvas. I think we end up with fewer conversions of colours under the hood.
  • updating the R3F camera manually on resize to (potentially) fix the pixel resizing issue 🎉
  • using the attach API for declaring buffer attributes
  • testing R3F components (the R3F test renderer has been available for a while, but it's worth keeping in mind)
  • suspend-react, a lower-level library for Suspense than react-suspense-fetch; it's developed by the R3F team and used internally in R3F itself

packages/lib/src/vis/shared/Html.tsx Outdated Show resolved Hide resolved
packages/lib/src/vis/shared/Html.tsx Outdated Show resolved Hide resolved
apps/demo/src/index.tsx Show resolved Hide resolved
@axelboc axelboc marked this pull request as draft May 13, 2022 13:31
@axelboc

This comment was marked as resolved.

@axelboc axelboc force-pushed the upgrade-react branch 3 times, most recently from 3105d9f to ebf8547 Compare June 3, 2022 14:57
@axelboc axelboc force-pushed the upgrade-react branch 3 times, most recently from 8de544e to 68618cc Compare June 8, 2022 08:10
@axelboc

This comment was marked as resolved.

@axelboc

This comment was marked as resolved.

@axelboc

This comment was marked as outdated.

@axelboc axelboc force-pushed the upgrade-react branch 2 times, most recently from 389741d to c24f283 Compare June 9, 2022 13:58
@axelboc

This comment was marked as outdated.

@bmaranville

This comment was marked as resolved.

@bmaranville

This comment was marked as resolved.

@bmaranville

This comment was marked as resolved.

@axelboc

This comment was marked as resolved.

@axelboc

This comment was marked as resolved.

@bmaranville

This comment was marked as resolved.

@axelboc

This comment was marked as resolved.

@axelboc
Copy link
Contributor Author

axelboc commented Feb 13, 2023

Attn @PeterC-DLS: I've just rebased this branch on v7. From now on, I'll try to rebase it only after stable releases.

I'm just doing a basic sanity check locally with pnpm start before force-pushing, and then the CI runs as usual—I'm not testing this branch in production projects yet—so do let me know if you notice issues.

@axelboc axelboc force-pushed the upgrade-react branch 2 times, most recently from db66ef4 to 2204322 Compare April 13, 2023 12:48
@axelboc axelboc force-pushed the upgrade-react branch 3 times, most recently from e63636e to dc7c275 Compare September 7, 2023 14:16
@axelboc
Copy link
Contributor Author

axelboc commented Sep 7, 2023

I've rebased and fixed a few things, but I had to skip a couple of tests for the CI to pass. I'll see if upgrading Testing Library helps and investigate a bit more.

The plan is to merge this and release a new major asap.

@axelboc
Copy link
Contributor Author

axelboc commented Sep 7, 2023

/approve

@axelboc axelboc force-pushed the upgrade-react branch 2 times, most recently from c895b68 to 157f7da Compare September 11, 2023 12:00
@axelboc axelboc marked this pull request as ready for review September 11, 2023 13:16
@axelboc axelboc requested a review from loichuder September 11, 2023 13:16
Copy link
Member

@loichuder loichuder left a comment

Choose a reason for hiding this comment

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

Looks all right to me.

Congrats for soldiering on through the timer issues. It should not have been easy.

packages/app/src/__tests__/Explorer.test.tsx Outdated Show resolved Hide resolved
@axelboc axelboc merged commit afe69e9 into main Sep 12, 2023
@axelboc axelboc deleted the upgrade-react branch September 12, 2023 11:11
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.

3 participants