Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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

Netlify deploy preview

https://deploy-preview-39795--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 4cec2fc

@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
@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

# for free to join this conversation on GitHub. Already have an account? # 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[material-ui][Autocomplete] Grouped options demo has console errors in Next.js
5 participants