Mojang / ore-ui

💎 Building blocks to construct game UIs using web tech.
https://react-facet.mojang.com/
MIT License
409 stars 18 forks source link

Update to React 18 #59

Open pirelenito opened 2 years ago

pirelenito commented 2 years ago

This PR updates the Facet packages to be based on React 18 (18.2.0 in specific), finally adding support to concurrent mode.

We are marking this PR for review, but will only proceed with merging after some internal testing within Mojang.

This PR has been a massive endeavor, with collaborations from @marlonicus and @ja-ni.

Release candidate

There is a new "release candidate" pushed in v0.6.0-rc.4, so you can help test this new release by getting the new release from npm:

"@react-facet/core": "^0.6.0-rc.4",

Deprecation of @react-facet/deferred-mount

The "Deferred Mount" solution was only needed as an alternative to "concurrent mode" while that wasn't available. So, this PR deletes the package from the repository, and we recommend any consumer to look at React 18's documentation and leverage their new APIs:

After merging, we will deprecate the package on NPM, and will be no longer maintaining it.

Effectless useFacetWrap, and new useFacetWrapMemo hook

In our internal codebase at Mojang, we rely a lot on useFacetWrap to keep components compatible with both regular values and Facets in their APIs. However while testing the improvements of this React 18 upgrade, we have identified that even though React makes the setup of the components concurrently, all the effects are actually run synchronously right after attaching the nodes to the DOM (as confirmed by this article):

Second, all effects (useInsertionEffect, useLayoutEffect, useEffect) run only after the render phase, which means that by the time they run, not only the component they’re being called in will have finished rendering, but also it will have been committed, which means that the whole component subtree for which the render was scheduled has finished rerendering.

So, to make sure this final synchronous step is smallest as possible (to avoid any frame spike), we have changed the default behaviour of the useFacetWrap hook so that it doesn't rely on an effect to update the wrapping value.

The main change this implies is that now if the wrapping value changes (is a different reference), a new Facet will be created.

As this change can have consequences down the rendering tree, there is also now a new component useFacetWrapMemo that retains the previous behaviour, ensuring that the resulting wrapped Facet is always the same and its just updated with new values.

On upgrading to this new version, we recommend profiling the application, and using the new hook as necessary to address performance regressions.

Now useFacetMemo can have an initial value

Previously the resulting Facet of calling useFacetMemo would always have NO_VALUE as its initial value, even if the Facet's it was dependent on had values.

This has been changed to help ensure that when React goes concurrently through a component tree, if a data is available, then it will be available at that time, and not at a later moment after all the effects were executed.

Changes to unit tests

Now that React no longer runs updates synchronously, any state update within a unit test must be wrapped in an await act:

await act(async () => {
  // perform state changes that would cause a component to update for testing purposes
})

The Map and Mount components are "deferred"

We have considered a good standard behaviour to have all "mount" operations to be considered low-priority and to have them run in concurrent mode.

From internal testing within Mojang, we believe this will give the smoothest experience to users of the UI.

This has no change in the API, and will be a transparent upgrade to any consumers of the library.

Updates to the benchmark test suite

With React 18 and concurrent mode, we had to do some fixes to the benchmark test suite to correctly measure and compare Facets with "Vanilla React". The main change revolves around getting the correct trace event FunctionCall, instead of just the FireAnimationEvent.

It also updates the tests to make sure we enable concurrent mode on the React 18 examples by using the createRoot API.

pirelenito commented 2 years ago

We can't do the update for now, as the type definitions are still not up-to-date. We could either wait for someone in the community to update, or maybe try to prepare a PR ourselves?

For now, I'm holding this PR until further notice.

pirelenito commented 2 years ago

New type definitions were published! So I took another stab at making this work.

Brute-forcing adding the new APIs as needed resulted in the examples working, but next step is understanding what and how we should use these new methods.

I've been using the https://github.com/pmndrs/react-three-fiber/blob/master/packages/fiber/src/core/renderer.ts as a reference implementation.

ja-ni commented 1 year ago

The React version 18.2.0 made some breaking changes to it's type definitions. The children property was removed from the FC type constructor but fortunately there is a migration script available to help update the existing type definitions:

https://github.com/eps1lon/types-react-codemod

This script does not completely alleviate the issue since for a number of our components we accept facets as children, in these instances the automatically generated types had to be adjusted.

Another issue that has arisen from the React 18 type changes is that the Element and ReactNode types are now misaligned with our current version of react-router. An upgrade of the react-router package is one solution to this issue but it is one that would require significant effort on planning.

CrossScarDev commented 5 months ago

this was last updated 2022. It's 2024