-
Notifications
You must be signed in to change notification settings - Fork 230
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
[DNM] React 19 #12374
[DNM] React 19 #12374
Conversation
<meta | ||
content="width=device-width, initial-scale=1, minimum-scale=1" | ||
data-react-helmet="true" | ||
name="viewport" | ||
/> | ||
<script |
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.
React 19 changes some behaviour of <head>
related tags, which likely accounts for this snapshot diff: https://react.dev/blog/2024/12/05/react-19#support-for-async-scripts
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 this is OK - it's just changing the order of the content in the snapshot by the looks of it?
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.
Yea I'll check and compare the ordering to see if there is any impact.
@@ -15,12 +15,14 @@ Cypress.Commands.add('hasNoscriptImgAtiUrl', atiUrl => { | |||
|
|||
// Should be moved into integration/pages/index.js once all pages have Chartbeat | |||
Cypress.Commands.add('hasScriptWithChartbeatSrc', () => { | |||
cy.reload(true); |
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.
Don't particularly like this, or the { retries: 3 }
on the individual Chartbeat tests, but I've tried everything to get them to work and couldn't find a better solution.
Feel like it may have something to do with how React 19 takes over <script />
tags, but can't be sure.
src/app/legacy/psammead/psammead-assets/src/amp-boilerplate.test.jsx
Outdated
Show resolved
Hide resolved
src/app/legacy/psammead/psammead-assets/src/amp-boilerplate.test.jsx
Outdated
Show resolved
Hide resolved
<meta | ||
content="width=device-width, initial-scale=1, minimum-scale=1" | ||
data-react-helmet="true" | ||
name="viewport" | ||
/> | ||
<script |
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 this is OK - it's just changing the order of the content in the snapshot by the looks of it?
@@ -39,7 +39,7 @@ const PlayButton = ({ | |||
type="button" | |||
css={styles.button} | |||
onClick={onClick} | |||
{...(className && { className })} | |||
{...(className ? { className } : 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.
So, basically this (and similar changes) are making sure we are following the correct spread pattern for React 19 because in 19 dev mode crashes when false is evaluated during spread?
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 didn't see any crashes as such, tests just failed quite spectacularly without these changes. Not to say it wouldn't cause the app to crash though, it very well could do without noticing as we still display the server render if the app crashes.
@@ -1,4 +1,4 @@ | |||
import React from 'react'; | |||
import React, { type JSX } from 'react'; |
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.
Do we still need to import React
here for storybook if we're removing it from imports in components below?
i.e in the Next app at ws-nextjs-app/pages/[service]/send/[id]/MessageBox/ErrorSummaryBox.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.
Yea we'll need to here. Other components that don't import will likely be importing something like:
/** @jsx jsx */
import { jsx } from '@emotion/react';
which means Emotion will be taking control of JSX rendering.
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.
Looks good and runs fine for me locally. Would be interesting to see what the affect on performance this upgrade might have immediately.
Took a capture of mundo if we want to do a pagespeed insights comparison:
https://pagespeed.web.dev/analysis/https-www-bbc-com-mundo/s30x4axjui?form_factor=mobile
Nice job man
This reverts commit 7529531.
Resolves JIRA https://jira.dev.bbc.co.uk/browse/WORLDSERVICE-139
Overall changes
react-is
version pin perhttps://github.com/jestjs/jest/issues/15402
https://github.com/facebook/react/issues/29902
. This seems to be a React issue with theclassic
runtime we're using. Updating toautomatic
runtime may fix this, but historically this broke Opera Mini, so this is a workaround to keep us on theclassic
runtimeTesting
Helpful Links
Add Links to useful resources related to this PR if applicable.
Coding Standards
Repository use guidelines