-
Notifications
You must be signed in to change notification settings - Fork 1k
Add support for react 19 #7532
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
Add support for react 19 #7532
Conversation
src/js/components/DataTable/__tests__/__snapshots__/DataTable-test.tsx.snap
Outdated
Show resolved
Hide resolved
@@ -91,20 +91,22 @@ const Text = forwardRef( | |||
</StyledText> | |||
); | |||
|
|||
const tipProps = tipProp && typeof tipProp === 'object' ? tipProp : {}; |
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 seems odd to me. If tipProp is defined but not an object why would we do {}? It seems like that loses tipProp.
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.
tipProp is either a string (which will be used for the content of the tooltip) or an object. If tipProp is a string then it doesn't make sense to pass that string as a prop to the Tooltip component. It should instead be passed as the child or through the content prop. This is why I set tipProps to an empty object if it is not an object.
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.
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.
Oh interesting, good callout
I was able to get our DS site upgraded to react 19 then pointing to this PR. Im looking through pages now make sure everything is running smooth so far so good.. but will update once Im finished |
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.
@jcfilben - Proposing the following summary for the React production mode test results: React19 Upgrade Performance SummaryOverviewGrommet would like to to support React19. In doing so, the following evaluation Summary findings
Methodology
Results
React 18{
"stats": {
"total": 53197.299999952316,
"reactVersion": "18.3.1-next-f1338f8080-20240426",
"durations": [
2589.899999976158,
2592.100000023842,
2605.399999976158,
2613.5,
2614.099999964237,
2615.800000011921,
2617,
2617.599999964237,
2622.300000011921,
2632.300000011921,
2635.300000011921,
2660.100000023842,
2665.199999988079,
2671.600000023842,
2687.800000011921,
2721.599999964237,
2739.399999976158,
2755.300000011921,
2755.5,
2785.5
],
"min": 2589.899999976158,
"max": 2785.5,
"mean": 2659.864999997616,
"median": 2635.300000011921,
"variance": 3507.965275049305,
"stdDev": 59.22807843455083,
"range": 195.60000002384186,
"mode": [
"2620",
4
]
}
} React 19{
"stats": {
"total": 48923.099999998696,
"reactVersion": "19.1.0",
"durations": [
2330.7999999998137,
2405.5999999996275,
2406.5,
2413.0999999996275,
2415.4000000003725,
2416.199999999255,
2417.699999999255,
2419,
2422,
2424.4000000003725,
2430.5,
2439.2000000001863,
2440.4000000003725,
2459.4000000003725,
2461.7999999998137,
2464.899999999441,
2465.4000000003725,
2465.5,
2562.2999999998137,
2663
],
"min": 2330.7999999998137,
"max": 2663,
"mean": 2446.1549999999347,
"median": 2430.5,
"variance": 4220.889475004651,
"stdDev": 64.96837288253917,
"range": 332.20000000018626,
"mode": [
"2420",
6
]
}
} ResourcesReact 19 slower in dev modeReact19 runs slower in dev mode due to several factors introduced to improve
Summary script// performance-benchmark.mjs
//
// From directory containing React DevTools Profiler JSON results,
// reads each JSON profile, summarizes, and writes summary stats to file.
import { argv } from 'node:process';
import { join } from 'node:path';
import { existsSync, promises as fsPromises } from 'node:fs';
import { parseArgs } from 'node:util';
const args = argv.slice(2);
const options = {
input: {
type: 'string',
short: 'i',
isRequired: true,
},
output: {
type: 'string',
short: 'o',
isRequired: true,
},
};
const { values } = parseArgs({
args,
options,
tokens: true,
});
const inputDirectory = values.input;
// Get parts from the input directory path
const inputDirectoryParts = inputDirectory
.split('/')
.filter((part) => part !== '');
const inputDirectoryName = inputDirectoryParts[inputDirectoryParts.length - 1];
const outputFile = values.output || `${inputDirectoryName}-results.json`;
const outputDirectory = join(inputDirectory, 'output');
// Check if the input directory exists
if (!existsSync(inputDirectory)) {
console.error(`Input directory "${inputDirectory}" does not exist.`);
process.exit(1);
}
// Read each file in the input directory
const files = await fsPromises.readdir(inputDirectory, { withFileTypes: true });
const inputFiles = files
.filter((file) => file.isFile() && file.name.endsWith('.json'))
.map((file) => file.name);
// Read each file and parse the JSON
const snapshots = await Promise.all(
inputFiles.map(async (file) => {
const adjFile = join(inputDirectory, file);
try {
const data = await fsPromises.readFile(adjFile, 'utf8');
const jsonObject = JSON.parse(data);
return jsonObject;
} catch (err) {
console.error(`Error processing file "${file}":`, err);
return null;
}
}),
);
const stats = snapshots
.map((snapshot) => {
const { timelineData } = snapshot;
const { duration, reactVersion } = timelineData[0];
return {
duration,
reactVersion,
};
})
.sort((a, b) => a.duration - b.duration)
.reduce(
(acc, curr) => {
acc.total += curr.duration;
acc.durations.push(curr.duration);
acc.reactVersion = curr.reactVersion;
return acc;
},
{
total: 0,
reactVersion: '',
durations: [],
},
);
stats.min = Math.min(...stats.durations);
stats.max = Math.max(...stats.durations);
stats.mean = stats.total / stats.durations.length;
stats.median = stats.durations[Math.floor(stats.durations.length / 2)];
stats.variance =
stats.durations.reduce((acc, curr) => acc + (curr - stats.mean) ** 2, 0) /
stats.durations.length;
stats.stdDev = Math.sqrt(stats.variance);
stats.range = stats.max - stats.min;
stats.mode = stats.durations.reduce((acc, curr) => {
const roundCurr = Math.round(curr / 10) * 10;
acc[roundCurr] = (acc[roundCurr] || 0) + 1;
return acc;
}, {});
stats.mode = Object.entries(stats.mode).reduce(
(acc, curr) => {
if (curr[1] > acc[1]) {
return curr;
}
return acc;
},
[0, 0],
);
console.log('Stats:', stats);
// Write the stats to the output file
const outputData = {
stats,
};
const outputDirectoryExists = existsSync(outputDirectory);
if (!outputDirectoryExists) {
await fsPromises.mkdir(outputDirectory, { recursive: true });
}
const outputFilePath = join(outputDirectory, outputFile);
const outputFileExists = existsSync(outputFilePath);
if (outputFileExists) {
console.log(`Output file "${outputFilePath}" already exists. Overwriting...`);
}
await fsPromises.writeFile(
outputFilePath,
JSON.stringify(outputData, null, 2),
'utf8',
);
console.log(`Output written to "${outputFilePath}"`);
console.log('Done!');
console.log('Performance benchmark completed.'); |
Co-authored-by: Matthew Glissmann <mdglissmann@gmail.com>
What does this PR do?
Add support for react 19
https://react.dev/blog/2024/04/25/react-19-upgrade-guide
I replaced places where we used
JSX.IntrinsicElements['div']
withReact.DetailedHTMLProps<React.HTMLAttributes<HTMLDivElement>, HTMLDivElement>
because in React 19 the JSX namespace moved to be under React rather than being global.React now gives an error when an empty string is passed to
src
so I changed some of the tests to resolve this.useRef() now requires a parameter, so there are some changes related to adding parameters to places where perviously we had none.
the
inert
prop is now recognized in react 19, so it can receive a value of true or false, whereas previously we needed to pass an empty string to set inert to true. I created theinertTrueValue
variable to handle the difference between react 19 and react <18.I made a few changes to the Tip component to manage the changes to refs that were made in React 19. (More details: https://react.dev/blog/2024/12/05/react-19#ref-as-a-prop)
I came across this issue that mentions potential performance issues with forwardRef in react 19. It might be good to test React 19 + this branch of Grommet in a large project just to see if there are any performance differences. facebook/react#31613
Where should the reviewer start?
What testing has been done on this PR?
Tested this branch with React 16, 17, 18, and 19 and confirmed they are all working.
I also did performance testing, see screenshots below.
How should this be manually tested?
Do Jest tests follow these best practices?
screen
is used for querying.asFragment()
is used for snapshot testing.Any background context you want to provide?
What are the relevant issues?
Closes #7458
Screenshots (if appropriate)
Performance testing on app with components from 'all' story
React 19 + this branch
average = 461.5
"timestamp": 490.4,





"timestamp": 521.5,
"timestamp": 418.5,
"timestamp": 447.7
"timestamp": 429.5,
React 18 + this branch
average = 499.1
"timestamp": 484.3,





"timestamp": 478.5,
"timestamp": 529.1,
"timestamp": 504.3,
"timestamp": 499.2,
React 18 + master
average = 531.7
"timestamp": 526.3,





"timestamp": 463.2,
"timestamp": 592.9,
"timestamp": 519.8,
"timestamp": 556.1,
Do the grommet docs need to be updated?
no
Should this PR be mentioned in the release notes?
yes
Is this change backwards compatible or is it a breaking change?
backwards compatible