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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

react 18.3.0 "A props object containing a "key" prop is being spread into JSX" #39833

Closed
Tracked by #42381
xaicron opened this issue Nov 11, 2023 · 16 comments 路 Fixed by #42689
Closed
Tracked by #42381

react 18.3.0 "A props object containing a "key" prop is being spread into JSX" #39833

xaicron opened this issue Nov 11, 2023 · 16 comments 路 Fixed by #42689
Assignees
Labels
bug 馃悰 Something doesn't work external dependency Blocked by external dependency, we can鈥檛 do anything about it

Comments

@xaicron
Copy link

xaicron commented Nov 11, 2023

Steps to reproduce 馃暪

Link to live example: https://mui.com/material-ui/react-autocomplete/#multiple-values

using Next.js

It works, but I get warnings on the console.

Warning: A props object containing a "key" prop is being spread into JSX:
  let props = {key: someKey, label: ..., size: ..., className: ..., disabled: ..., data-tag-index: ..., tabIndex: ..., onDelete: ...};
  <ForwardRef(Chip) {...props} />
React keys must be passed directly to JSX without using spread:
  let props = {label: ..., size: ..., className: ..., disabled: ..., data-tag-index: ..., tabIndex: ..., onDelete: ...};
...

Current behavior 馃槸

Some of the issues have been identified and addressed at #39474, and they have already been resolved. However, a similar problem occurs with renderTags when using multiple.

Expected behavior 馃

It appears that the following code also needs to be modified:
https://github.com/mui/material-ui/blob/v5.14.17/packages/mui-material/src/Autocomplete/Autocomplete.js#L506-L525

Also, the type of the props for renderOption is currently defined as React.HTMLAttributes, but in reality, it includes the key property. This might cause some confusion in TypeScript.

Context 馃敠

No response

Your environment 馃寧

npx @mui/envinfo

  System:
    OS: macOS 14.1
  Binaries:
    Node: 20.9.0 - ~/.volta/tools/image/node/20.9.0/bin/node
    Yarn: 1.22.19 - ~/.volta/tools/image/yarn/1.22.19/bin/yarn
    npm: 10.2.3 - ~/.volta/tools/image/npm/10.2.3/bin/npm
  Browsers:
    Chrome: 119.0.6045.123
    Edge: Not Found
    Safari: 17.1
  npmPackages:
    @emotion/react: ^11.11.1 => 11.11.1
    @emotion/styled: ^11.11.0 => 11.11.0
    @mui/base:  5.0.0-beta.23
    @mui/core-downloads-tracker:  5.14.17
    @mui/icons-material: ^5.14.16 => 5.14.16
    @mui/lab: ^5.0.0-alpha.152 => 5.0.0-alpha.152
    @mui/material: ^5.14.17 => 5.14.17
    @mui/private-theming:  5.14.17
    @mui/styled-engine:  5.14.17
    @mui/system:  5.14.17
    @mui/types:  7.2.8
    @mui/utils:  5.14.17
    @mui/x-date-pickers: ^6.18.1 => 6.18.1
    @types/react: ^18.2.37 => 18.2.37
    react: ^18.2.0 => 18.2.0
    react-dom: 18.2.0 => 18.2.0
    typescript: ^5.2.2 => 5.2.2

@xaicron xaicron added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Nov 11, 2023
@oliviertassinari oliviertassinari added the component: autocomplete This is the name of the generic UI component, not the React module! label Nov 11, 2023
@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 11, 2023

If it's a Next.js regression, maybe it's for them to fix it, but maybe not depending on what they say about it. If they say they will fix it in 4 weeks, I don't think it's worth it for us, if it's "normal", 馃憤 to handle.

At least it's a tradeoff, we are adding complexity to the codebase that is a good benefit to 1/4 of the users (Next.js userbase?) and harms a little bit 3/4.

@ZeeshanTamboli ZeeshanTamboli changed the title [material-ui][Autocomplete] When usgin multipule option has console errors in Next.js [material-ui][Autocomplete] When using multiple option has console errors in Next.js Nov 11, 2023
@xaicron
Copy link
Author

xaicron commented Nov 14, 2023

Thank you for your quick reply! And, thank you for commenting on the Next.js issue.
It indeed seems to be an issue with Next.js, and I agree that it should be addressed on the Next.js side rather than MUI's.

@mj12albert mj12albert added external dependency Blocked by external dependency, we can鈥檛 do anything about it nextjs and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Nov 20, 2023
@nicobatalla
Copy link

This is not only in Next.js. It also happens with React starting from version 18.3.1 when not using Next.js

@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari added bug 馃悰 Something doesn't work and removed external dependency Blocked by external dependency, we can鈥檛 do anything about it nextjs labels May 8, 2024
@oliviertassinari oliviertassinari changed the title [material-ui][Autocomplete] When using multiple option has console errors in Next.js react 18.3.0 "A props object containing a "key" prop is being spread into JSX" May 8, 2024
@oliviertassinari oliviertassinari removed the component: autocomplete This is the name of the generic UI component, not the React module! label May 8, 2024
@oliviertassinari
Copy link
Member

oliviertassinari commented May 8, 2024

I had a closer look at this. The behavior of the JSX transform is interesting:

const profile1 = (
  <div key="baarr"{...foo} />
);

const profile2 = (
  <div {...foo} key="baarr" />
);

outputs:

import { jsx as _jsx } from "react/jsx-runtime";
import { createElement as _createElement } from "react";

const profile1 = /*#__PURE__*/_jsx("div", {
  ...foo
}, "baarr");

const profile2 = /*#__PURE__*/_createElement("div", {
  ...foo,
  key: "baarr"
});

Babel REPL

Now, when you check the sources, I'm super confused: https://github.com/facebook/react/blob/04b058868c9fc61c78124b12efb168734d79d09e/packages/react/src/jsx/ReactJSXElement.js#L557-L562. But it gets a lot clearer from: https://github.com/reactjs/rfcs/blob/createlement-rfc/text/0000-create-element-changes.md

The goal is to bring element creation down to this logic:

function jsx(type, props, key) {
  return {
    $$typeof: ReactElementSymbol,
    type,
    key,
    props,
  };
}

So what React wants is a clear key prop, so the Babel plugin can pass it to the right argument. This key needs to be applied first, React then relies on TypeScript to flag duplicate keys when the spread has a key property. Smart

SCR-20240509-cpzg

I have created #42168 to illustrate it.

@oliviertassinari oliviertassinari added the external dependency Blocked by external dependency, we can鈥檛 do anything about it label May 8, 2024
@DiegoAndai
Copy link
Member

DiegoAndai commented May 9, 2024

Thanks for taking a deeper look @oliviertassinari!


Summarizing, if I understand correctly

1. key must go first so the jsx function is used.

The reason <div {...foo} key="baarr" /> outputs the following:

_createElement("div", { ...foo, key: "baarr" });

instead of using jsx as follows:

// jsx signature: (type, config, maybeKey)
_jsx("div", { ...foo }, "baarr")

is because in jsx, config.key (foo.key) would override maybeKey ("baarr") (reference), which is not expected when <div {...foo} key="baarr" />.

It seems like this issue could go away when they stop pulling foo.key 馃, doesn't it?

2. key must be removed from props to be ready for future changes and remove the warning.

This is because even though the jsx function pulls it it at the moment (reference), it won't do it forever (reference). This is where the warning is coming from (reference). This warning will not be removed even when they stop pulling key from it (reference).


Please correct me if this summary is incorrect.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 9, 2024

@DiegoAndai This matches what I understand of the situation 馃憤

@amd2107
Copy link

amd2107 commented May 29, 2024

Hi, I've been having this issue using Next.js 14 + React 18 + MUI 5.15

This error comes up when you select an item and the Chip appears within the input, not sure if you already solve it but I was able to solve it overriding the Chip with the renderTags:

            <Autocomplete
              multiple
              ...
              renderTags={(value, getTagProps) =>
                value.map((option, index) => {
                  const { key, ...tagProps } = getTagProps({ index });
                  return <Chip variant={'outlined'} color={'primary'} size={"small"} key={key} label={option.title} {...tagProps} />;
                })
              }
            />

Basically you cannot pass the {...props} containing a key prop so you need to destructure key from props

For example:

              renderOption={(props, option, { selected }) => {
                const { key, ...rest } = props;
                return (
                  <li key={key} {...rest}>
                     ...
                  </li>
                );
              }}

Hope it works to other folks!

@mbiggs-gresham
Copy link

I raised an issue #42161 which got closed as a duplicate of this one. However, the fixes that went in to v5.15.19 don't seem to address it so i'm hoping it will still be tracked and resolved.

Whilst as @amd2107 points out, there is a key prop passed within the props object to the renderOptions, the typescript has no mention of it it seems, meaning you have to cast the props yourself.

@DiegoAndai are there plans to address this still?

@a-tonchev
Copy link

a-tonchev commented Jun 6, 2024

I have the same issue - React + mui, mostly with the Autocomplete
I had to do unnecessary renderTags to solve it:

            renderTags={(value, getTagProps) => value.map((option, index) => {
              const tagProps = getTagProps({ index });
              return (
                <Chip
                  label={option.name}
                  {...tagProps}
                  key={option.id}
                />
              );
            })}            

@DiegoAndai
Copy link
Member

DiegoAndai commented Jun 6, 2024

I raised an issue #42161 which got closed as a duplicate of this one. However, the fixes that went in to v5.15.19 don't seem to address it so i'm hoping it will still be tracked and resolved.

[...]

@DiegoAndai are there plans to address this still?

Yes, I'll take a look as soon as possible, sorry for the delay. I'll keep you posted.

@amd2107
Copy link

amd2107 commented Jun 6, 2024

@mbiggs-gresham @a-tonchev you should upgrade to the following version:

npm i @mui/material@6.0.0-alpha.10 --save or npm i @mui/material@next --save

I spent several hours exploring how MUI works behind the scenes. While testing in the integrated playground, I upgraded to Next.js 14, and the error was resolved. I believe the issue has been fixed in this latest version.

Hope it works!

@oliviertassinari @DiegoAndai

This topic can be closed as the error has been resolved in the mentioned version.

image

Edit: I removed the part where I mentioned compatibility with version 7 and other MUI plugins. Initially, it seemed to work, but after rebuilding the project and fixing the component styles, it resulted in a failure. Note that there are no compatible grid or time picker libraries yet for this next version.

@mohit2530
Copy link

I was also seeing this issue in react and I am not using Nextjs.. I was also able to resolve the issue by de-structing the object that is being passed into the renderOption with muiv5. Basically, the key prop must be removed before passing into the <li> elements. For me, it was like -

  renderOption={(props, option) => {
                const { key, ...rest } = props;
                return (
                  <li key={key} {...rest}>
                    {option.location}
                  </li>
                );
              }}

The answer above from @amd2107 and @a-tonchev helped me. Hope it helps others as well.

@DiegoAndai
Copy link
Member

DiegoAndai commented Jun 18, 2024

An update. This issue covers two related bugs:

The first bug; key spreading inside the Autocomplete component codebase, was fixed in #42099 and is available from versions v5.15.19 and v6.0.0-alpha.7 onwards.

To close this issue, we need to fix the second one: renderOptions prop's props argument, which is incorrectly typed as React.HTMLAttributes<HTMLLIElement> missing the key property, forcing people to cast it as reported in this issue as well as in #42161. Sorry for the delay @mbiggs-gresham.

Other components might have similar issues to the one I mentioned above. We should look for and fix those in the same PR. This is included in the support React 19 work, so it should be picked up in the coming weeks.

Thank for your patience.

@mbiggs-gresham
Copy link

Thanks for the update

@DiegoAndai
Copy link
Member

The renderOption type issue has been fixed in #42689 and cherry-picked in #42709. The fix will be out on next week's release.

Please let us know if you encounter the same issue again or a similar one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 馃悰 Something doesn't work external dependency Blocked by external dependency, we can鈥檛 do anything about it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants