-
Notifications
You must be signed in to change notification settings - Fork 145
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
feat(react 18): upgrade to react 18. #7336
base: main
Are you sure you want to change the base?
feat(react 18): upgrade to react 18. #7336
Conversation
Merging latest changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/tests/end-to-end/tests/details-view/fast-pass-report.test.ts
Outdated
Show resolved
Hide resolved
src/tests/unit/tests/DetailsView/components/assessment-instance-table.test.tsx
Outdated
Show resolved
Hide resolved
src/tests/unit/tests/DetailsView/components/assessment-instance-table.test.tsx
Outdated
Show resolved
Hide resolved
src/tests/unit/tests/DetailsView/components/details-view-command-bar.test.tsx
Outdated
Show resolved
Hide resolved
src/tests/unit/tests/DetailsView/no-content-available-view-renderer.test.tsx
Outdated
Show resolved
Hide resolved
src/tests/unit/tests/popup/components/launch-panel-header.test.tsx
Outdated
Show resolved
Hide resolved
src/tests/unit/tests/DetailsView/no-content-available-view-renderer.test.tsx
Show resolved
Hide resolved
src/tests/unit/tests/popup/components/diagnostic-view-toggle.test.tsx
Outdated
Show resolved
Hide resolved
src/tests/unit/tests/popup/components/diagnostic-view-toggle.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is a huge effort! Well done, team! I have some questions about changes, mostly in tests.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 exportmockReactComponent
- use
mockReactComponent(Dialog, 'Dialog')
in tests that have a snapshot that uses theDialog
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have implemented suggested changes.
@@ -11,7 +11,7 @@ exports[`NextRequirementButton renders 1`] = ` | |||
exports[`NextRequirementButton renders: CustomizedDefaultButton props 1`] = ` | |||
{ | |||
"children": <span> | |||
<Memo(Icon) | |||
<Memo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can avoid this generic component name in the snapshot by mocking the memoized Icon
component:
mockReactComponents([(Icon as any).type]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have implemented suggested changes.
@@ -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 () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have implemented suggested changes.
@@ -98,11 +103,6 @@ describe('SaveAssessmentButton', () => { | |||
fireEvent.click(wrapper.getByRole('link')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should every fireEvent
call in this file be wrapped with act
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still have this open question
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fireEvent wrapped with act
@@ -35,6 +35,7 @@ describe('StoresTree', () => { | |||
|
|||
const props = { | |||
deps, | |||
state: undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? I don't think it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
|
||
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())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.setup(r => r(It.isAny())) | |
.setup(r => r(fakeDocument.getElementById('popup-container'))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
@@ -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'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the changes.
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
old code, can be deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
src/views/insights/initializer.ts
Outdated
@@ -69,7 +69,7 @@ const documentManipulator = new DocumentManipulator(document); | |||
const rendererDependencies: RendererDeps = { | |||
textContent, | |||
dom: document, | |||
render: ReactDOM.render, | |||
render: createRoot, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
…akeshsh/accessibility-insights-web into v-rakeshsh/react18-migration
@@ -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`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
@@ -98,11 +103,6 @@ describe('SaveAssessmentButton', () => { | |||
fireEvent.click(wrapper.getByRole('link')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still have this open question
.setup(render => | ||
render( | ||
createRootMock | ||
.setup(r => r(It.isAny())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.setup(r => r(It.isAny())) | |
.setup(r => r(It.is((container: any) => container != null))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
|
||
const wrapper = render(component); | ||
const toggle = wrapper.getByRole('switch'); | ||
fireEvent.focus(toggle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't need the fireEvent
or rerender
calls here since our new class is setting the properties for initial render.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
|
||
const renderMock: any = Mock.ofType<typeof createRoot>(); | ||
createRootMock | ||
.setup(r => r(It.isAny())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this change in the code.
it('dialog is hidden (dismissed) when "got it" button is clicked', async () => { | ||
const gotItButtonProps = getMockComponentClassPropsForCall(PrimaryButton); | ||
gotItButtonProps.onClick(); | ||
const getProps = getMockComponentClassPropsForCall(Dialog); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is checking the props from the initial render.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
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`; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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();
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The props snapshot needs a snapshot name for accuracy:
expectMockedComponentPropsToMatchSnapshots([Dialog], 'Dialog props');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The props snapshots need a snapshotName
for accuracy:
expectMockedComponentPropsToMatchSnapshots([Dialog], 'Dialog props');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -65,7 +57,9 @@ describe('SaveAssessmentButton', () => { | |||
describe('render', () => { | |||
beforeEach(() => { | |||
wrapper = render(<SaveAssessmentButton {...propsStub} />); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
wrapper = render(<SaveAssessmentButton {...propsStub} />); | |
mockReactComponent(Dialog, 'Dialog'); | |
wrapper = render(<SaveAssessmentButton {...propsStub} />); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test also needs to mock (Icon as any).type
in order to not have the generic Memo
in the snapshot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this change Snapshot is not changing generic Memo
is still there in the snapshot. Even if we try to do mockReactComponent((Icon as any).type, 'Icon')
it gives error as mock-Icon is not a mockable component. Please add a jest.mock call for this component before using this component in the test function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Details
This feature updates below packages.
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);
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
WrappedComponent: React.ComponentType
,
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.
>.
3. Notable changes for @testing-library/react:
4. Notable changes for @fluentui/react from v8.x.x to v8.118.1
5. Notable changes for react-helmet-async:
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
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.
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
yarn fastpass
yarn test
)<rootDir>/test-results/unit/coverage
fix:
,chore:
,feat(feature-name):
,refactor:
). SeeCONTRIBUTING.md
.