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

Update to React 18 #59

Open
wants to merge 81 commits into
base: main
Choose a base branch
from
Open

Update to React 18 #59

wants to merge 81 commits into from

Conversation

pirelenito
Copy link
Member

@pirelenito pirelenito commented Apr 27, 2022

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 pirelenito changed the title Updates to React 18 Update to React 18 Apr 27, 2022
@pirelenito
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

ja-ni commented Dec 7, 2022

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.

pirelenito and others added 22 commits November 14, 2023 16:16
Although I believe we should review these rules in a follow-up PR
React 18 will no longer have its work happening under the `FireAnimationFrame`

So instead we track all the `FunctionCall`, which is then valid for both implementations
React 18 does do multiple "FunctionCall" events on a single frame, making it impossible to compare when sampling the same amount.

So, we rely solely on time to compare both implementations.

We've also increased the time before starting collecting data
…onent

So, we need to ensure that when wrapping a facet, we do its updates within an effect.
Given its deprecated, we will handle it internally where needed.
This hook is used extensively in our internal codebase at Mojang. So, introducing an effect resulted in a very expensive performance cost.

The new implementation has a disadvantage that it will re-create a facet when the wrapping value changes. But from our measurements it is a valid tradeoff.
The former would "block the browser from painting", which is unnecessary for these hooks.

More info: https://react.dev/reference/react/useLayoutEffect#examples
@CrossScarDev
Copy link

this was last updated 2022. It's 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.

4 participants