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.

Sign up for GitHub

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

Merged
merged 5 commits into from
May 8, 2024
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f34fd0e
Fixed React 18.3 warning about spreading key in the `AutoComplete` co…
heath-freenome May 2, 2024
5882451
Add missing cases
DiegoAndai May 2, 2024
8b3ab3c
Update packages/mui-joy/src/Autocomplete/Autocomplete.test.tsx
heath-freenome May 2, 2024
604fead
Reverse unnecessary change
DiegoAndai May 6, 2024
ceeb9fd
Merge branch 'next' into fix-autocomplete-to-remove-react18.3-key-spr…
DiegoAndai May 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion packages/mui-base/src/useAutocomplete/useAutocomplete.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,12 @@ describe('useAutocomplete', () => {
{groupedOptions.length > 0 ? (
<ul {...getListboxProps()}>
{groupedOptions.map((option, index) => {
return <li {...getOptionProps({ option, index })}>{option}</li>;
const { key, ...optionProps } = getOptionProps({ option, index });
return (
<li key={key} {...optionProps}>
{option}
</li>
);
})}
</ul>
) : null}
Expand Down
20 changes: 12 additions & 8 deletions packages/mui-material/src/Autocomplete/Autocomplete.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,14 +537,18 @@ const Autocomplete = React.forwardRef(function Autocomplete(inProps, ref) {
if (renderTags) {
startAdornment = renderTags(value, getCustomizedTagProps, ownerState);
} else {
startAdornment = value.map((option, index) => (
<Chip
label={getOptionLabel(option)}
size={size}
{...getCustomizedTagProps({ index })}
{...ChipProps}
/>
));
startAdornment = value.map((option, index) => {
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 •
edited
Loading

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.

label={getOptionLabel(option)}
size={size}
{...customTagProps}
{...ChipProps}
/>
);
});
}
}

Expand Down
5 changes: 4 additions & 1 deletion packages/mui-material/src/Autocomplete/Autocomplete.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,7 +638,10 @@ describe('<Autocomplete />', () => {
renderTags={(value, getTagProps) =>
value
.filter((x, index) => index === 1)
.map((option, index) => <Chip label={option.title} {...getTagProps({ index })} />)
.map((option, index) => {
const { key, ...tagProps } = getTagProps({ index });
return <Chip key={key} label={option.title} {...tagProps} />;
})
}
onChange={handleChange}
renderInput={(params) => <TextField {...params} autoFocus />}
Expand Down
Loading