feat(react 18): upgrade to react 18. #7336

v-rakeshsh · 2024-05-15T11:26:17Z

Details

This feature updates below packages.

react from v16 to v18.
react-dom from v16 to v18.
@types-react from v16 to v18.
@types-react-dom from v16 to v18.
@testing-library/react from v12 to v15.
@fluentui/react from v8.x.x to v8.118.1.
Removed react-helmet and added react-helmet-async.

1. Notable changes for react, react-dom:

Motivation: React 18 introduces a new root API which provides better ergonomics for managing roots. The new root API also enables the new concurrent renderer, which allows you to opt-into concurrent features.

In V16, we had below to render the component:
import { render } from 'react-dom';
const container = document.getElementById('app');
render(, container);

In V18, we have below to render the component:
import { createRoot } from 'react-dom/client';
const container = document.getElementById('app');
const root = createRoot(container); // createRoot(container!) if you use TypeScript
root.render();

2. Notable changes for @types-react and @types-react-dom:

Motivation: The new types are safer and catch issues that used to be ignored by the type checker. The most notable change is that the children prop now needs to be listed explicitly when defining props

In old we have below
WrappedComponent: React.ComponentType
,
In new we have below
WrappedComponent: React.ComponentType<React.PropsWithChildren
>,

Approach for type changes: So this Type changes are added using automation script https://github.com/eps1lon/types-react-codemod. This automation script is suggested in react18 migration document.

Added new package types-react-codemod.
After adding the package, executed yarn types-react-codemod preset-18 ./src in root, and then selected all option from the list of options.
This will transform all types of component type having child components to <React.PropsWithChildren
>.

3. Notable changes for @testing-library/react:

Current version of @testing-library/react does not support react18, so from v13.x.x, react18 support is added. So updated to latest V15. For reference - https://github.com/testing-library/react-testing-library/releases/tag/v13.0.0
Wrapped state updates/async operations under act.
Updated test cases with createRoot for createRootMock instead of render and renderMock.

4. Notable changes for @fluentui/react from v8.x.x to v8.118.1

Existing fluent ui version does not support react18, test cases were failing, hence after checking v8.118.1 documentation, it supports react and react-dom v18. Hence upadated.

5. Notable changes for react-helmet-async:

Current react-helmet package throws error 'objects cannot be child, expected elements', for react18, Hence as alternative used react-helmet-async. For reference https://www.npmjs.com/package/react-helmet-async?activeTab=readme because react-helmet-async uses react18 as dependency.
Wrapped Helmet provider for root, as to pass context of react-helmet-async.
Created a variable to store data, and then this data was passed as JSX, instead of passing the data as it is. Because it will throw "Objects cannot be used as react elements".

For example:
export const GuidanceTitle = NamedFC<GuidanceTitleProps>('GuidanceTitle', ({ name }) => { const titleValue = Guidance for ${name} - ${productName}; return ( <> <Helmet> <title>{titleValue}</title> </Helmet> <h1>{name}</h1> </> ); });

6. Along with above

Made changes to mock helpers, because after react18 changes, the JSON structure of component was coming differently, so accordingly corrected the helpers, to get proper component name for snapshots.
Updated snapshots, because as we are using latest Fluent UI version, new props are introduced which can be seen in snapshots.
Refactored few test cases, which were wrong logically, like for example:
using of mockReactComponents in global and inside test case using of useOriginalComponents to get the props using getMockComponentClassPropsForCall which was wrong logically is fixed to use any one approach.
Updated report package with react, react-dom v18 to keep in sync with AI web.

Context

This PR includes all changes required for migration of AI web from react16 to react18.
It includes test cases fixes.
It includes lint issues fixes.

Pull request checklist

Addresses an existing issue: #0000
Ran yarn fastpass
Added/updated relevant unit test(s) (and ran yarn test)
Verified code coverage for the changes made. Check coverage report at: <rootDir>/test-results/unit/coverage
PR title AND final merge commit title both start with a semantic tag (fix:, chore:, feat(feature-name):, refactor:). See CONTRIBUTING.md.
(UI changes only) Added screenshots/GIFs to description above
(UI changes only) Verified usability with NVDA/JAWS

Merging latest changes

v-viyada

Please update the details for each type of change we did in PR description. Overall PR looks good but provided few nit pick comments. Please check for other similar changes.

src/popup/incompatible-browser-renderer.tsx

src/tests/end-to-end/tests/details-view/fast-pass-report.test.ts

src/tests/unit/tests/DetailsView/components/assessment-instance-table.test.tsx

src/tests/unit/tests/DetailsView/components/details-view-command-bar.test.tsx

src/tests/unit/tests/DetailsView/no-content-available-view-renderer.test.tsx

src/tests/unit/tests/popup/components/launch-panel-header.test.tsx

src/tests/unit/tests/DetailsView/no-content-available-view-renderer.test.tsx

src/tests/unit/tests/popup/components/diagnostic-view-toggle.test.tsx

madalynrose

This PR is a huge effort! Well done, team! I have some questions about changes, mostly in tests.

madalynrose · 2024-05-28T15:10:51Z

.../unit/tests/DetailsView/components/__snapshots__/auto-detected-failures-dialog.test.tsx.snap

@@ -10,7 +10,7 @@ exports[`AutoDetectedFailuresDialog on componentDidUpdate does not display dialo
 exports[`AutoDetectedFailuresDialog on dialog enabled box appears checked when "dont show again" box is clicked 1`] = `
 <body>
  <div>
-    <mock-dialog
+    <mock-styledwithresponsivemode


I'm unsure what caused the Dialog component to render with a different name, but I think it's worth it to:

update mock-module-helpers.tsx to export mockReactComponent

use mockReactComponent(Dialog, 'Dialog') in tests that have a snapshot that uses the Dialog component:

this file

generic-dialog.test.tsx

export-dialog.test.tsx

invalid-load-assessment-dialog.test.tsx

issue-filing-doalog.test.tsx

quick-assess-to-assessment-dialog.test.tsx

save-assessment-button.test.tsx

details-dialog.test.tsx

We have implemented suggested changes.

madalynrose · 2024-05-28T15:17:03Z

src/tests/unit/tests/DetailsView/components/__snapshots__/next-requirement-button.test.tsx.snap

@@ -11,7 +11,7 @@ exports[`NextRequirementButton renders 1`] = `
 exports[`NextRequirementButton renders: CustomizedDefaultButton props 1`] = `
 {
  "children": <span>
-    <Memo(Icon)
+    <Memo


We can avoid this generic component name in the snapshot by mocking the memoized Icon component:

mockReactComponents([(Icon as any).type]);

We have implemented suggested changes.

madalynrose · 2024-05-28T15:45:21Z

src/tests/unit/tests/DetailsView/components/save-assessment-button.test.tsx

@@ -81,6 +80,12 @@ describe('SaveAssessmentButton', () => {
                });
                expect(getMockComponentClassPropsForCall(Dialog, 3).hidden).toEqual(true);
            });
+            it('dialog is hidden (dismissed) when "got it" button is clicked', async () => {


This test no longer tests the same functionality when placed in this describe block (especially when testing the props from the initial render). It should be moved back to the 'interaction' describe block.

We have implemented suggested changes.

madalynrose · 2024-05-28T15:47:46Z

src/tests/unit/tests/DetailsView/components/save-assessment-button.test.tsx

@@ -98,11 +103,6 @@ describe('SaveAssessmentButton', () => {
                fireEvent.click(wrapper.getByRole('link'));


should every fireEvent call in this file be wrapped with act ?

still have this open question

fireEvent wrapped with act

madalynrose · 2024-05-28T15:57:32Z

src/tests/unit/tests/debug-tools/components/stores-tree.test.tsx

@@ -35,6 +35,7 @@ describe('StoresTree', () => {

            const props = {
                deps,
+                state: undefined,


Is this necessary? I don't think it is.

madalynrose · 2024-05-30T17:47:27Z

src/tests/unit/tests/popup/main-renderer.test.tsx


        const popupWindowMock = Mock.ofInstance(window);
        const hasAccess = true;
        const targetTabUrl = 'url';

        const deps: MainRendererDeps = Mock.ofType<MainRendererDeps>().object;
+        const createRootMock: any = Mock.ofType<typeof createRoot>();
+        createRootMock
+            .setup(r => r(It.isAny()))


Suggested change

.setup(r => r(It.isAny()))

.setup(r => r(fakeDocument.getElementById('popup-container')))

madalynrose · 2024-05-30T17:58:09Z

src/tests/unit/tests/reports/components/inline-image.test.tsx

@@ -46,7 +46,7 @@ describe('InlineImageTest', () => {
        const renderResult = render(<InlineImage {...props} />);
        expect(renderResult.container.firstChild).not.toBeNull();

-        const element = renderResult.getByRole('img', { name: alt }) as HTMLImageElement;
+        const element = renderResult.container.querySelector('img');


getByRole('img') fails for the first test case because its alt is empty and therefore it is assigned role="presentation". Rather than using querySelector, we can plan for this:

Suggested change

const element = renderResult.container.querySelector('img');

const element = !isEmpty(alt) ? renderResult.getByRole('img', { name: alt }) as HTMLImageElement : renderResult.getByRole('presentation') as HTMLImageElement;

This will also require importing isEmpty from lodash

Updated the changes.

madalynrose · 2024-05-30T17:58:35Z

src/tests/unit/tests/popup/main-renderer.test.tsx

@@ -30,17 +30,25 @@ describe('MainRenderer', () => {
        const diagnosticViewToggleFactoryMock = Mock.ofType(DiagnosticViewToggleFactory);
        const dropdownClickHandlerMock = Mock.ofType(DropdownClickHandler);

-        const renderMock: IMock<typeof ReactDOM.render> = Mock.ofInstance(() => null);
+        //const renderMock: IMock<typeof createRoot> = Mock.ofInstance(() => null);


old code, can be deleted.

madalynrose · 2024-05-30T18:04:18Z

src/views/insights/initializer.ts

@@ -69,7 +69,7 @@ const documentManipulator = new DocumentManipulator(document);
 const rendererDependencies: RendererDeps = {
    textContent,
    dom: document,
-    render: ReactDOM.render,
+    render: createRoot,


I think it'd be worth renaming this to be createRoot everywhere (in this initializer and others and the renderers) now that it's a two-step process.

madalynrose · 2024-05-30T18:59:38Z

src/reports/components/report-instance-list.tsx

These changes do away with one custom type (Cell) entirely and render another custom type basically unused (Row). Is there any way to preserve their usage?

…akeshsh/accessibility-insights-web into v-rakeshsh/react18-migration

madalynrose · 2024-06-06T18:01:30Z

src/tests/unit/mock-helpers/mock-module-helpers.tsx

@@ -9,7 +9,7 @@ export const expectMockedComponentPropsToMatchSnapshots = (
    components.forEach(component => {
        const componentSnapshotName =
            snapshotName ||
-            `${component.displayName || component.name || 'mocked component'} props`;
+            `${component.displayName || component.name || component?.render?.displayName || 'mocked component'} props`;


We need to calculate the component name the same way we do in mockReactComponent below in order to have it respect the passed in name.

madalynrose · 2024-06-06T18:13:18Z

src/tests/unit/tests/DetailsView/components/save-assessment-button.test.tsx

@@ -98,11 +103,6 @@ describe('SaveAssessmentButton', () => {
                fireEvent.click(wrapper.getByRole('link'));


still have this open question

madalynrose · 2024-06-06T18:15:33Z

src/tests/unit/tests/injected/dialog-renderer-impl.test.tsx

-            .setup(render =>
-                render(
+        createRootMock
+            .setup(r => r(It.isAny()))


Suggested change

.setup(r => r(It.isAny()))

.setup(r => r(It.is((container: any) => container != null)))

madalynrose · 2024-06-06T18:18:36Z

src/tests/unit/tests/popup/components/diagnostic-view-toggle.test.tsx

+
+            const wrapper = render(component);
+            const toggle = wrapper.getByRole('switch');
+            fireEvent.focus(toggle);


We shouldn't need the fireEvent or rerender calls here since our new class is setting the properties for initial render.

madalynrose · 2024-06-06T18:25:22Z

src/tests/unit/tests/popup/incompatible-browser-renderer.test.tsx

-
+        const renderMock: any = Mock.ofType<typeof createRoot>();
+        createRootMock
+            .setup(r => r(It.isAny()))


I don't see this change in the code.

madalynrose · 2024-06-06T18:59:32Z

src/tests/unit/tests/DetailsView/components/save-assessment-button.test.tsx

+            it('dialog is hidden (dismissed) when "got it" button is clicked', async () => {
+                const gotItButtonProps = getMockComponentClassPropsForCall(PrimaryButton);
+                gotItButtonProps.onClick();
+                const getProps = getMockComponentClassPropsForCall(Dialog);


This is checking the props from the initial render.

madalynrose · 2024-06-07T14:41:10Z

src/tests/unit/mock-helpers/mock-module-helpers.tsx

+        let componentSnapshotName;
+
+        componentSnapshotName = component.displayName
+            ? `${component.displayName} props`
+            : `${component.name} props`;
+
+        if (snapshotName) {
+            componentSnapshotName = snapshotName;
+        }
+
+        if (componentSnapshotName === 'undefined props') {
+            componentSnapshotName = component?.render?.displayName
+                ? `${component?.render?.displayName} props`
+                : `mocked component props`;
+        }
+


I was mistaken! I forgot that the snapshot name needs to be passed in for Dialog to be correctly listed in the snapshot name. So this can go back to how it was and I'll make suggestions to fix the snapshot names in the appropriate files.

Suggested change

let componentSnapshotName;

componentSnapshotName = component.displayName

? `${component.displayName} props`

: `${component.name} props`;

if (snapshotName) {

componentSnapshotName = snapshotName;

}

if (componentSnapshotName === 'undefined props') {

componentSnapshotName = component?.render?.displayName

? `${component?.render?.displayName} props`

: `mocked component props`;

}

const componentSnapshotName =

snapshotName ||

`${component.displayName || component.name || 'mocked component'} props`;

madalynrose · 2024-06-07T14:49:36Z

src/tests/unit/tests/DetailsView/components/export-dialog.test.tsx

The props snapshots need a snapshot name ("Dialog props") passed in:

it.each(isOpenOptions)('with open %p', isOpen => { props.isOpen = isOpen; onlyIncludeHtmlService(); const renderResult = render(<ExportDialog {...props} />); expectMockedComponentPropsToMatchSnapshots([Dialog], 'Dialog props'); expect(renderResult.asFragment()).toMatchSnapshot(); });

madalynrose · 2024-06-07T14:52:28Z

src/tests/unit/tests/DetailsView/components/invalid-load-assessment-dialog.test.tsx

The props snapshot needs a snapshot name for accuracy:

expectMockedComponentPropsToMatchSnapshots([Dialog], 'Dialog props');

madalynrose · 2024-06-07T14:54:32Z

src/tests/unit/tests/DetailsView/components/issue-filing-dialog.test.tsx

The props snapshots need a snapshotName for accuracy:

expectMockedComponentPropsToMatchSnapshots([Dialog], 'Dialog props');

madalynrose · 2024-06-07T15:01:04Z

src/tests/unit/tests/DetailsView/components/save-assessment-button.test.tsx

@@ -65,7 +57,9 @@ describe('SaveAssessmentButton', () => {
        describe('render', () => {
            beforeEach(() => {
                wrapper = render(<SaveAssessmentButton {...propsStub} />);


evidently we need to mock the Dialog component much closer to this actual test case in order for the snapshot to reflect the correct name.

Suggested change

wrapper = render(<SaveAssessmentButton {...propsStub} />);

mockReactComponent(Dialog, 'Dialog');

wrapper = render(<SaveAssessmentButton {...propsStub} />);

madalynrose · 2024-06-07T16:25:57Z

...it/tests/DetailsView/components/tab-stops-requirement-instances-collapsible-content.test.tsx

This test also needs to mock (Icon as any).type in order to not have the generic Memo in the snapshot

madalynrose · 2024-06-07T16:29:45Z

src/tests/unit/tests/common/components/__snapshots__/telemetry-permission-dialog.test.tsx.snap

The corresponding test file for this also needs to call mockReactComponent(Dialog, 'Dialog') to preserve the name in the snapshot. And will need to call

expectMockedComponentPropsToMatchSnapshots([Dialog], 'Dialog');

separately from the call for the other components on line 64 in order to preserve the snapshot name.

v-rakeshsh and others added 18 commits April 12, 2024 10:42

react DOM and react version changes only

2ee56c1

Merge branch 'microsoft:main' into v-rakeshsh/react18-migration

0e1825b

reverted deletion of snapshot

ca62789

Merge branch 'microsoft:main' into main

b33dbc3

feature:test cases fix after react18 updates

294a4c6

feature:test cases fix after react18 updates

7662bc1

Merge branch 'microsoft:main' into main

99fdc7b

feature:test cases fix after react18 updates

ff28c7f

Merge branch 'main' into v-rakeshsh/react18-migration

975d1c9

Merging latest changes

feature:test cases fix after react18 updates

fac7061

feature:test cases fix after react18 updates

26c1e7d

failing test cases fixes

1932ca5

failing test cases fixes

446e661

failing test cases fixes

e7f6d77

merge latest main

93dd327

merge latest changes

2c5928b

mock component test files changes

c4e26ad

merge latest changes

bbc267e

v-rakeshsh requested a review from a team as a code owner May 15, 2024 11:26

v-rakeshsh marked this pull request as draft May 15, 2024 11:26

v-rakeshsh and others added 10 commits May 15, 2024 17:04

chore: update react18 versions and its dependencies

efb002f

chore: update react18 versions and its dependencies

28b23e8

chore: update react18 versions and its dependencies

7c00a38

chore: update react18 versions and its dependencies

1bfae5d

chore: update react18 versions and its dependencies

be59511

React 18 review comment changes

57a8f66

React 18 changes & yarn.lock revert

9f5ff81

Build Issue,Mock,Version - Fixes

a13cacb

Merge branch 'main' into v-rakeshsh/react18-migration

bdffcb6

resolving conflict yarn.lock

29159b5

Fixing null check error

5e1389a

v-viyada reviewed May 22, 2024

View reviewed changes

v-sharmachir and others added 3 commits May 23, 2024 11:50

Resolving review comments

ceb0c5e

removing commented code

ccd3048

Update tab-stops-requirement-instances-collapsible-content.test.tsx

f040941

v-viyada marked this pull request as ready for review May 24, 2024 16:38

v-viyada changed the title ~~V rakeshsh/react18 migration~~ feat(react 18): upgrade to react 18. May 24, 2024

madalynrose requested changes May 30, 2024

View reviewed changes

SaanicaG and others added 8 commits May 31, 2024 11:11

Merge branch 'microsoft:main' into v-rakeshsh/react18-migration

7d704b9

mocked Dialog and icon

001dadc

shifted one test case into interaction block

15c329f

PR Review Comments - Fixes

2976f72

Merge branch 'v-rakeshsh/react18-migration' of https://github.com/v-r…

f6c7c03

…akeshsh/accessibility-insights-web into v-rakeshsh/react18-migration

Comments resolution

f3aac39

removed unneccessary act

2440f2d

fixed lint error

a563eb8

madalynrose requested changes Jun 6, 2024

View reviewed changes

comment Resolution

ae8f8db

madalynrose requested changes Jun 7, 2024

View reviewed changes

		@@ -98,11 +103,6 @@ describe('SaveAssessmentButton', () => {
		fireEvent.click(wrapper.getByRole('link'));

	.setup(r => r(It.isAny()))
	.setup(r => r(fakeDocument.getElementById('popup-container')))

	const element = renderResult.container.querySelector('img');
	const element = !isEmpty(alt) ? renderResult.getByRole('img', { name: alt }) as HTMLImageElement : renderResult.getByRole('presentation') as HTMLImageElement;

	.setup(r => r(It.isAny()))
	.setup(r => r(It.is((container: any) => container != null)))

	wrapper = render(<SaveAssessmentButton {...propsStub} />);
	mockReactComponent(Dialog, 'Dialog');
	wrapper = render(<SaveAssessmentButton {...propsStub} />);

feat(react 18): upgrade to react 18. #7336

Are you sure you want to change the base?

feat(react 18): upgrade to react 18. #7336

Conversation

v-rakeshsh commented May 15, 2024 • edited

Details

Context

Pull request checklist

v-viyada left a comment

Choose a reason for hiding this comment

madalynrose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

v-rakeshsh commented May 15, 2024 •

edited