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

[Autocomplete][material-ui] Fix React key warning in Next.js #39795

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

mj12albert
Copy link
Member

@mj12albert mj12albert commented Nov 8, 2023

Closes #39474
Also see #39833 as related (for our demos in the docs that have a renderOption)

Demo: https://codesandbox.io/p/sandbox/https-github-com-mui-material-ui-pull-39795-fwdm7f?file=%2Fsrc%2Fapp%2Fpage.tsx%3A1%2C1

There should no console errors when opening the listbox with grouped options

Next.js doesn't like it when a key prop is part of an object being spread, here's the related issue in their repo: vercel/next.js#55642

BTW I'm not sure why this doesn't repro when the options are ungrouped, as in both cases they both use defaultRenderOption

@mj12albert mj12albert added typescript package: material-ui Specific to @mui/material component: autocomplete This is the name of the generic UI component, not the React module! nextjs labels Nov 8, 2023
@mui-bot
Copy link

mui-bot commented Nov 8, 2023

@mj12albert mj12albert marked this pull request as ready for review November 8, 2023 07:57
@mj12albert mj12albert requested review from michaldudak, DiegoAndai and mnajdova and removed request for DiegoAndai November 8, 2023 07:58
@mnajdova mnajdova merged commit 13fec2d into mui:master Nov 8, 2023
24 checks passed
@mj12albert mj12albert deleted the material/autocomplete-grouped branch November 8, 2023 10:09
Comment on lines +555 to +562
const defaultRenderOption = (props2, option) => {
const { key, ...otherProps } = props2;
return (
<li key={key} {...otherProps}>
{getOptionLabel(option)}
</li>
);
};
Copy link
Member

@oliviertassinari oliviertassinari Nov 11, 2023

Choose a reason for hiding this comment

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

The spread is not a very fast operator (speaking under @romgrk control, who I think did changes like this in the MUI X codebase to improve performance)

Would this be better?

Suggested change
const defaultRenderOption = (props2, option) => {
const { key, ...otherProps } = props2;
return (
<li key={key} {...otherProps}>
{getOptionLabel(option)}
</li>
);
};
const defaultRenderOption = (props2, option) => {
// Need to clearly apply key because of https://github.com/vercel/next.js/issues/55642
return (
<li key={props2.key} {...props2}>
{getOptionLabel(option)}
</li>
);
};
Copy link
Member

Choose a reason for hiding this comment

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

Handled in #40968

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: autocomplete This is the name of the generic UI component, not the React module! nextjs package: material-ui Specific to @mui/material typescript
5 participants