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

[core] Fix React 18.3 warnings about spreading keys in the Material UI Autocomplete component #42099

Conversation

heath-freenome
Copy link
Contributor

@heath-freenome heath-freenome commented May 2, 2024

This is related to #40905 only it fixes @mui/material

Iterate toward the resolution of #39833.

@mui-bot
Copy link

mui-bot commented May 2, 2024

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against ceeb9fd

@DiegoAndai DiegoAndai self-requested a review May 2, 2024 20:59
@DiegoAndai DiegoAndai changed the title Fixed React 18.3 warning about spreading key in the AutoComplete component [core] Fix React 18.3 warnings about spreading keys in the AutoComplete component May 2, 2024
@DiegoAndai
Copy link
Member

Hey @heath-freenome, thanks for working on this! We need this for #42047, but it's missing some fixes. I've updated the PR with the missing cases. Adding @ZeeshanTamboli as a reviewer.

@DiegoAndai DiegoAndai requested review from ZeeshanTamboli and removed request for DiegoAndai May 2, 2024 21:11
@DiegoAndai DiegoAndai self-assigned this May 2, 2024
@DiegoAndai DiegoAndai added core Infrastructure work going on behind the scenes component: autocomplete This is the name of the generic UI component, not the React module! labels May 2, 2024
@DiegoAndai DiegoAndai mentioned this pull request May 2, 2024
15 tasks
@DiegoAndai
Copy link
Member

@michaldudak This requires an update on mui-base, but it's only a test file, so I figure it's no problem, right?

@heath-freenome
Copy link
Contributor Author

Hey @heath-freenome, thanks for working on this! We need this for #42047, but it's missing some fixes. I've updated the PR with the missing cases. Adding @ZeeshanTamboli as a reviewer.

@DiegoAndai Thanks for fixing that!!

@michaldudak
Copy link
Member

This requires an update on mui-base, but it's only a test file, so I figure it's no problem, right?

No problem at all! I'll port this update to the Base UI repo as well.

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@heath-freenome Thank you for the PR. Would you like this PR to also address the issue for @mui/joy Autocomplete and thereby close #40905? Given the error in #40905 (comment), I believe it relates to the Autocomplete option. Handling the separate passing of the key for selected Chip is already taken care of in Joy UI.

packages/mui-joy/src/Autocomplete/Autocomplete.test.tsx Outdated Show resolved Hide resolved
const { key, ...customTagProps } = getCustomizedTagProps({ index });
return (
<Chip
key={key}
Copy link
Member

Choose a reason for hiding this comment

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

Should we consider #39795 (comment) here?

Copy link
Member

Choose a reason for hiding this comment

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

We could do

startAdornment = value.map((option, index) => {
  const customTagProps = getCustomizedTagProps({ index });
  const key = customTagProps.key;
  delete customTagProps.key;
  return (
    <Chip
      key={key}
      label={getOptionLabel(option)}
      size={size}
      {...customTagProps}
      {...ChipProps}
    />
  );
});

or

startAdornment = value.map((option, index) => {
  const customTagProps = getCustomizedTagProps({ index });
  return (
    <Chip
      label={getOptionLabel(option)}
      size={size}
      {...customTagProps}
      key={customTagProps.key} // specifying it after stops the warning
      {...ChipProps}
    />
  );
});

Are there other options? I'm not sure which one I like the least 😅

Also, I'm not sure if we will get the benefits with option 2: facebook/react#25697

I couldn't find much info about this warning 😓

Copy link
Member

Choose a reason for hiding this comment

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

Option 2 is similar to what was done in #40968 (See #39795 (comment)). However, I also believe it won't make much difference, as you mentioned in facebook/react#25697 (comment). So, the current solution seems fine.

Copy link
Member

@oliviertassinari oliviertassinari May 8, 2024

Choose a reason for hiding this comment

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

I think #39795 (comment) is wrong in retrospect considering how the Babel plugin is implemented: #39833 (comment). It could have worked, but it doesn't. React is relying on TypeScript to help detect key duplicates.

@ZeeshanTamboli ZeeshanTamboli added package: material-ui Specific to @mui/material core Infrastructure work going on behind the scenes and removed core Infrastructure work going on behind the scenes labels May 4, 2024
@ZeeshanTamboli ZeeshanTamboli changed the title [core] Fix React 18.3 warnings about spreading keys in the AutoComplete component [core] Fix React 18.3 warnings about spreading keys in the Autocomplete component May 4, 2024
@ZeeshanTamboli ZeeshanTamboli removed the package: material-ui Specific to @mui/material label May 4, 2024
@michaldudak michaldudak added the port-to-base-ui PR to be included in the Base UI repository once the API changes are done label May 7, 2024
@ZeeshanTamboli ZeeshanTamboli added the package: material-ui Specific to @mui/material label May 8, 2024
@ZeeshanTamboli ZeeshanTamboli changed the title [core] Fix React 18.3 warnings about spreading keys in the Autocomplete component [core] Fix React 18.3 warnings about spreading keys in the Material UI Autocomplete component May 8, 2024
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

Looks good. If we prefer not to address Joy UI Autocomplete in this PR (#42099 (review)), it can be handled separately. I've updated the title and label accordingly to focus on Material UI.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 8, 2024
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! core Infrastructure work going on behind the scenes package: material-ui Specific to @mui/material port-to-base-ui PR to be included in the Base UI repository once the API changes are done React 19 support PRs required to support React 19
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants