Skip to content

[utils] skip deep clone React element#44400

Merged
siriwatknp merged 2 commits intomui:masterfrom
siriwatknp:fix/merge-theme
Nov 18, 2024
Merged

[utils] skip deep clone React element#44400
siriwatknp merged 2 commits intomui:masterfrom
siriwatknp:fix/merge-theme

Conversation

@siriwatknp
Copy link
Copy Markdown
Member

@siriwatknp siriwatknp commented Nov 13, 2024

closes #44278

Root cause

Next.js 15 with Turbopack has a different signature for React element which cause the @mui/utils/deepmerge to recursively deepclone React element. More details on #44278

Fix

I think it does not make sense anyway for the deepmerge function to deepClone React elements.

So I added a check to deepmerge directly to avoid deepClone React elements.

Tested with https://github.com/nphmuller/mui-next-15_0_2_issue, this PR fixes the issue.


@siriwatknp siriwatknp added type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. nextjs labels Nov 13, 2024
@mui-bot
Copy link
Copy Markdown

mui-bot commented Nov 13, 2024

Netlify deploy preview

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

@material-ui/core: parsed: -0.10% 😍, gzip: -0.05% 😍

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 34690d5

Copy link
Copy Markdown
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.

Skipping React elements in deepClone and deepMerge makes sense to me 👍🏼

Because this is a shared util, let's wait to see if @mui/x or @mui/toolpad have any objections. If there are none in a couple of days, we can go ahead and merge this.

Does that sound right?

@flaviendelangle
Copy link
Copy Markdown
Member

We are not using either of those (unless they are used under the hood in some API we use) so for me it's good

@bharatkashyap
Copy link
Copy Markdown
Collaborator

We're using it in two (1, 2) places but both of those are for merging theme objects deeply so this should not impact us 👍🏻

@siriwatknp
Copy link
Copy Markdown
Member Author

Thanks for the info @flaviendelangle @bharatkashyap

Copy link
Copy Markdown
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.

@siriwatknp looks like we're good to go, let's merge 😊

@siriwatknp siriwatknp merged commit d153daa into mui:master Nov 18, 2024
@bordeo
Copy link
Copy Markdown

bordeo commented Nov 20, 2024

@siriwatknp can you merge this fix into mui v5? There's the same issue.
Thank you

jukkatupamaki pushed a commit to jukkatupamaki/material-ui that referenced this pull request Nov 21, 2024
jukkatupamaki pushed a commit to jukkatupamaki/material-ui that referenced this pull request Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

nextjs type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Next 15: createTheme(): RangeError: Maximum call stack size exceeded

6 participants