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

[material-ui][Autocomplete] Fix more React 18.3 key spread warnings in demos #42639

Merged
merged 5 commits into from
Jun 26, 2024

Conversation

wbt
Copy link
Contributor

@wbt wbt commented Jun 13, 2024

Note: the PR has expanded to solve all errors in all Autocomplete demos, not only the Google Maps demo as the original PR description explained.


When using the demo code for autocomplete with Google Maps, there is an error thrown which seems to be discussed in other issues with respect to other broken examples, but I don't see a solution in place yet.
This addresses the Google Maps example only. There are probably other examples that should be updated too, but getting this merged seems like a positive step forward even if there are other positive steps forward that can follow.

Examples of issues closed as incomplete without a solution that seem to be reporting a similar error, especially with NextJS and React 18.3+:

This is still open:

The issue appears to have been partially addressed in

but this particular issue is still in the latest, so here's a step to fix it.

This borrows from #39833 (comment) in place of a slightly less elegant earlier draft.

@mui-bot
Copy link

mui-bot commented Jun 13, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 18520bf

@wbt
Copy link
Contributor Author

wbt commented Jun 13, 2024

It looks like I can't add a label on the PR as is required by test, and the GitHub editor doesn't apply prettier settings.

@zannager zannager added the component: autocomplete This is the name of the generic UI component, not the React module! label Jun 14, 2024
@zannager zannager requested a review from mj12albert June 14, 2024 13:31
@danilo-leal danilo-leal changed the title Fix error in Google Maps example [material-ui][Autocomplete] Fix error in Google Maps demo Jun 14, 2024
@danilo-leal danilo-leal added docs Improvements or additions to the documentation package: material-ui Specific to @mui/material labels Jun 14, 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.

@wbt Can you provide a live reproduction first of the error in CodeSandbox or StackBlitz?

@wbt
Copy link
Contributor Author

wbt commented Jun 19, 2024

It looks like that requires setting up a special billing account, getting an API key, and possibly breaking pseudonymity in open-source contributions more broadly just to re-demonstrate an issue that has several existing reports already. The existing example code does not need to be modified to demonstrate the issue; it appears that using it as-is in a project with React 18.3+ and your own API key would likely suffice to show it, assuming that NONE of the other issue reports appear credible.

For quick setup close to the context where I saw it, I recommend running npx create-next-app@latest in an empty directory, with default options. Add the original and/or modified file affected by this PR into /components, paste in your API key on line 13, and then import/add it onto the default page.tsx that you get with the new Next app. Make sure your key allows requests from http://localhost:3000 or has no referrer restriction.

@m4theushw
Copy link
Member

The demo in https://mui.com/material-ui/react-autocomplete/#globally-customized-options also has the same problem with the key prop. I was playing with passing a custom renderOption to Autocomplete and saw the error.

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Jun 20, 2024

I saw other Autocomplete demos were updated in #42691. Maybe we need to update this one and this one as well? I'll leave this PR to be reviewed by @aarongarciah and @DiegoAndai since they have more context.

@aarongarciah aarongarciah removed the request for review from mj12albert June 20, 2024 10:42
@aarongarciah aarongarciah added the on hold There is a blocker, we need to wait label Jun 20, 2024
@aarongarciah
Copy link
Member

We'll hold this PR until #42689 is merged so we get the correct types.

Note that React 18.3.0 started to throw warnings when spreading key. It's not Material UI specific (see https://github.com/facebook/react/releases/tag/v18.3.0). We're fixing the issues as we find them.

This error, and the one in GloballyCustomizedOptions.tsx mentioned by @m4theushw, were harder to catch than the ones fixed in #42691 because these warnings only happen at runtime and the Google Maps demo didn't throw the warning until there are results, which is not the case if you don't have an API key 😅

I see that there are more warnings that we didn't catch because they are fired when the autocomplete is open and renderOption is called. I'll push the fixes to this PR so we merge them all at once.


@wbt when a demo is modified (the .tsx file), you need to run pnpm docs:typescript to generate the corresponding .js file.

@aarongarciah
Copy link
Member

aarongarciah commented Jun 20, 2024

Just pushed a commit that should solve all React warnings related to spreading key in Autocomplete demos. The test_types CI check will fail until we rebase on to of next once #42689 is merged.

I recommend reviewing this PR with the "Hide whitespace" option enabled.

See how to enable it Screenshot 2024-06-20 at 13 41 51

@aarongarciah aarongarciah changed the title [material-ui][Autocomplete] Fix error in Google Maps demo [material-ui][Autocomplete] Fix more React 18.3 key spread warnings in demos Jun 20, 2024
@aarongarciah aarongarciah requested review from mnajdova and removed request for DiegoAndai June 20, 2024 11:47
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.

@aarongarciah I hope you don't mind the review. I was just scrolling through and found this.

@aarongarciah aarongarciah removed the on hold There is a blocker, we need to wait label Jun 24, 2024
@aarongarciah aarongarciah requested review from DiegoAndai and removed request for mnajdova June 24, 2024 14:15
@aarongarciah
Copy link
Member

aarongarciah commented Jun 24, 2024

I just rebased to get the latest typing fixes from #42689

@aarongarciah aarongarciah added needs cherry-pick The PR should be cherry-picked to master after merge React 19 support PRs required to support React 19 labels Jun 24, 2024
@aarongarciah aarongarciah self-assigned this Jun 24, 2024
Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

@aarongarciah aarongarciah merged commit 21c95e7 into mui:next Jun 26, 2024
22 checks passed
github-actions bot pushed a commit that referenced this pull request Jun 26, 2024
…n demos (#42639)

Signed-off-by: wbt <wbt@users.noreply.github.com>
Co-authored-by: Aarón García Hervás <aaron@mui.com>
@wbt wbt deleted the fix-google-maps-props-spread branch June 26, 2024 21:25
joserodolfofreitas pushed a commit to joserodolfofreitas/material-ui that referenced this pull request Jul 29, 2024
…n demos (mui#42639)

Signed-off-by: wbt <wbt@users.noreply.github.com>
Co-authored-by: Aarón García Hervás <aaron@mui.com>
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! docs Improvements or additions to the documentation needs cherry-pick The PR should be cherry-picked to master after merge package: material-ui Specific to @mui/material React 19 support PRs required to support React 19
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants