Upgrade to FE to use React 18

[AbhiPrasad]

Problem Statement

Investigate

Beta Give feedback

1. feat(js): Upgrade to React 18 #54686
   Scope: Frontend Stale +2

Tasks

Beta Give feedback

1. Upgrade to React 18
2. Upgrade react-testing-library v13 https://github.com/testing-library/react-testing-library/releases/tag/v13.0.0
3. Switch to new react-dom methods
4. Squash The current testing environment is not configured to support act errors

https://react.dev/blog/2022/03/29/react-v18

Upgrade guide: https://react.dev/blog/2022/03/08/react-18-upgrade-guide

We'll get:

• useTransition and some other new hooks
• Perf improvements with batching
• Ability to use suspense if we want to

Solution Brainstorm

To investigate

• Updating react testing library
• Check strict mode changes
• Check if HMR still works
• Check if types still work
• Explore using useSyncExternalStore

Product Area

Unknown

[AbhiPrasad]

Sample try when just bumping deps: #54686

test failures: https://github.com/getsentry/sentry/actions/runs/5857714436/

TypeError: Cannot read properties of undefined (reading 'current')

It seems that reactHooks.renderHook (from @testing-library/react-hooks) returns an undefined current.

ref https://github.com/testing-library/react-hooks-testing-library#a-note-about-react-18-support, it seems that renderHook is not supported with React 18 and @testing-library/react-hooks at all (even if you use the old renderer)

We probably need to swap to import {renderHook} from '@testing-library/react', but that requires us to upgrade testing library the same time we do react.

An alternative is to swap out all usage of renderHook with our own internal helper (that just vendors in what is in testing-library/react-testing-library#991)

FAIL  static/app/views/replays/detail/network/useSortNetwork.spec.tsx
● useSortNetwork › should the list by timestamp by default

  TypeError: Cannot read properties of undefined (reading 'current')

    94 |     },
    95 |   }),
  > 96 |   TestStubs.Replay.RequestFrame({
       |                                         ^
    97 |     op: 'resource.fetch',
    98 |     description: 'https://pokeapi.co/api/v2/pokemon/mewtu',
    99 |     startTimestamp: new Date(1663131120.198),

    at act (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:15259:59)
    at render (node_modules/@testing-library/react-hooks/lib/native/pure.js:73:34)
    at Object.renderHook (node_modules/@testing-library/react-hooks/lib/core/index.js:114:5)
    at Object.<anonymous> (static/app/views/replays/detail/network/useSortNetwork.spec.tsx:96:41)

Unable to find an element with the text: Nested Container - nested

It seems that expect(await screen.findByText('X')).toBeInTheDocument(); and similar does not work in certain scenarios. Not sure when/why exactly though.

FAIL  static/app/components/events/eventViewHierarchy.spec.tsx
● Event View Hierarchy › does not collapse all nodes when update triggers re-render

  Unable to find an element with the text: Nested Container - nested. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.

  Ignored nodes: comments, <script />, <style />
  ...

    91 |       }
    92 |     );
  > 93 |
       | ^
    94 |     expect(await screen.findByText('Nested Container - nested')).toBeInTheDocument();
    95 |
    96 |     rerender(<EventViewHierarchy project={mockProject} event={event} />);

    at waitForWrapper (node_modules/@testing-library/dom/dist/wait-for.js:187:27)
    at node_modules/@testing-library/dom/dist/query-helpers.js:101:33
    at Object.<anonymous> (static/app/components/events/eventViewHierarchy.spec.tsx:93:46)

Timed out in waitForElementToBeRemoved

Some timeouts, but this might just be a flake, nothing confirmed.

FAIL  static/app/views/onboarding/setupDocs.spec.tsx (7.118 s)
● Onboarding Setup Docs › renders Product Selection › only error monitoring checked

  Timed out in waitForElementToBeRemoved.

  Ignored nodes: comments, <script />, <style />
  ...

    267 |       renderMockRequests({
    268 |         project,
  > 269 |         orgSlug: organization.slug,
        |                                                                ^
    270 |       });
    271 |
    272 |       render(

    at waitForElementToBeRemoved (node_modules/@testing-library/dom/dist/wait-for-element-to-be-removed.js:22:24)
    at Object.<anonymous> (static/app/views/onboarding/setupDocs.spec.tsx:269:64)

[scttcper]

I think in TSC we talked about using 18 with the legacy renderer and migrating after to the new react 18 renderer. This will break the migration into easier chunks since we can get both repos to react 18 and the test updates and then migrate to the new renderer (and probably fix tests again).

Proposed path:

• Update to react 18, @types/react and RTL latest version (14)
  • do not switch to the new createRoot
  • pass legacyRoot to the test renderer
• Migrate to the new createRoot and the async renderer, remove legacyRoot
  • migrate tests again as necessary

React Hook test migration

Shim @testing-library/react-hooks

@testing-library/react-hooks is now exported directly from RTL. We'll need to export an object that has a similar shape until we migrate all the hook tests to importing renderHook directly.

// In sentry-test/reactTestingLibrary
export const reactHooks = {renderHook: rtl.renderHook, act: rtl.act, waitFor: rtl.waitFor};

Hooks using rerender() need to have props

they no longer keep their initial props

const {result, rerender} = reactHooks.renderHook(useTimeout, {
      initialProps: {timeMs, onTimeout},
});
// Before
rerender()
// After
rerender({timeMs, onTimeout})

Swap waitForNextUpdate for waitFor

should be straight forward. waitForNextUpdate is removed

Remove the returned waitFor from reactHooks.renderHook

waitFor is no longer returned, use the regular one

const {result, waitFor} = reactHooks.renderHook

Figure out how to catch errors? Maybe skip these tests

In some react hook tests we throw an error and check result.error. This no longer works and we'll need to do something else. Maybe skip the tests until then, mostly low value

sentry/static/app/components/replays/useReplaysCount.spec.tsx

Lines 31 to 38 in 79bb00c

	it('should throw if none of groupIds, replayIds, transactionNames is provided', () => {
	const {result} = reactHooks.renderHook(useReplaysCount, {
	initialProps: {
	organization,
	},
	});
	expect(result.error).toBeTruthy();
	});

Remove act from hook tests

it stops working in the next version, should be using waitFor probably
https://github.com/getsentry/sentry/blob/master/static/app/views/replays/detail/network/useSortNetwork.spec.tsx#L178-L180

Component test migration

Change RTL jest import

Instead of '@testing-library/jest-dom/extend-expect', in the jest.config. add import '@testing-library/jest-dom'; to the test setup.

Ignore react 18 warning

in setupFramework.ts add

  if (
    /Warning: ReactDOM.render is no longer supported in React 18/.test(errorMessage)
  ) {
    return true;
  }

Use a regular date mock jest.useFakeTimers().setSystemTime

Some tests are using fake timers to mock the date, this is not how we should be freezing the date since it breaks timeouts etc. We have a date mock library we can switch to.

sentry/static/app/views/dashboards/widgetBuilder/widgetBuilderDataset.spec.tsx

Line 403 in 79bb00c

jest.useFakeTimers().setSystemTime(new Date('2022-08-02'));

Fix various act errors

@AbhiPrasad covered this. Need to use more await screen.findBy in tests to silence act warnings that were previously hidden.

fix various waitForElementToBeRemoved

@AbhiPrasad covered this. just swap these for waitFor(() => expect(screen.queryBy()).not.toBeInTheDocument()) or remove

Other things to do

• bump emotion, there's some react 18 type conflicts that are fixed in the latest verison
• Add children?: React.ReactNode where necessary
• Tons of type fixes like withSentryRouter probably needs a return type
• type needs to have children added to it

here's where i just kinda mashed a bunch together https://github.com/getsentry/sentry/pull/57987/files