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

Upgrade to react 18 rendering api #1246

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

Conversation

andrewHEguardian
Copy link
Contributor

@andrewHEguardian andrewHEguardian commented Nov 10, 2023

What does this change?

Upgrade to using react 18 rendering API - createRoot see docs. We've been on React 18 for a while but running as if React 17. This will remove the console message seen below:

image

How to test

Site behaves as expected, no regressions in Cypress tests, Chromatic etc...

See Risks section for what happened...

How can we measure success?

Have we considered potential risks?

Had to add flushSync around an event handler in HolidayCalendarTables.tsx to get HolidayStop Cypress to pass. The component behaved normally when manually testing but the state was not updating correctly when running in Cypress, I think because of the improved batching in React 18

Image of Cypress test failing without flushSync, dates should be from 9/2 to 11/2 but the state is set with 9/2 to 9/2
image

Images

Accessibility

@andrewHEguardian andrewHEguardian force-pushed the ahe/use-react-18-rendering-api branch from 1cea47d to 0f4cabe Compare November 14, 2023 12:37
KentGrm
KentGrm previously approved these changes Nov 22, 2023
@@ -103,7 +103,8 @@ describe('Holiday stops', () => {

cy.findByText('No issues occur during selected period').should('exist');

cy.get('@product_detail.all').should('have.length', 1);
// ToDo: why is this being called more times?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you worked this out yet?

client/MMAPage.ts Outdated Show resolved Hide resolved
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.

3 participants