[Tabs] Replace cloneElement with Context API to support custom and wrapped Tab components
#46333
Conversation
ZeeshanTamboli
added
component: tabs
ZeeshanTamboli
changed the title
Netlify deploy previewhttps://deploy-preview-46333--material-ui.netlify.app/ Bundle size report@mui/material parsed: 🔺+438B(+0.08%) gzip: 🔺+208B(+0.14%) Show details for 100 more bundles (86 more not shown)@mui/material/Tab parsed: 🔺+370B(+0.56%) gzip: 🔺+177B(+0.78%) |
ZeeshanTamboli
changed the title
ZeeshanTamboli
changed the title
cloneElement with Context API to support custom and wrapped Tab components
| if (!hasRegisteredRef.current) { | ||
| hasRegisteredRef.current = true; |
There was a problem hiding this comment.
function block inside useState hook runs only once through out component lifecycle (it doesn't run when component re-renders). So curious to understand the purpose of hasRegisteredRef, as it will always be false when this code runs
There was a problem hiding this comment.
You're right that the initializer inside useState only runs once per mount. However, in React’s Strict Mode (development only), it's intentionally called twice to detect impure logic. Since registerTab mutates internal state, calling it twice would incorrectly register the tab multiple times, shifting tab indices and breaking the selection or indicator logic.
To avoid this, we guard it with hasRegisteredRef, ensuring registerTab runs only once — even in development. In production, the guard has no effect because the initializer runs only once as expected.
Ideally, the initializer should be pure (per React docs), but we intentionally break that rule here to support SSR — specifically, to precompute tab metadata so that the correct tab is marked selected on the first render (see test case). Without it, we get hydration mismatches..
I considered making registerTab idempotent, but that’s not feasible when we need to assign an implicit value based on the tab's render order. That requires incrementing a shared index counter (childIndexRef) — and we can’t require users to always provide explicit values to Tab without introducing a breaking change.
This approach strikes a balance: it ensures SSR correctness, avoids hydration issues, and works with wrapper components like <Tooltip><Tab /></Tooltip>, while remaining safe under React’s development behavior.
Open to suggestions if you think there's a cleaner way to achieve this.
There was a problem hiding this comment.
If I understand it correctly this won't work well if you remove a tab dynamically (as there's no unregister function)
There was a problem hiding this comment.
If I understand it correctly this won't work well if you remove a tab dynamically (as there's no unregister function)
It won't. But it isn't supported even in latest version.
This PR: https://stackblitz.com/edit/ry4fan5c-t3b4771r
Master: https://stackblitz.com/edit/ry4fan5c-oqugmytq
There was a problem hiding this comment.
Using the register pattern without an unregister function sounds to me like it can introduce weird edge cases. Did you consider registering/unregistering on an effect instead?
There was a problem hiding this comment.
Registering during an effect phase will prevent it from running on the server. Perhaps we could register both during rendering and in an effect and make the register operation idempotent (or register conditionally if it hasn't been registered yet). This will allow the use of the unregister function in the effect cleanup.
There was a problem hiding this comment.
Yes, registering during the effect phase would prevent it from running on the server, which breaks SSR.
Perhaps we could register both during rendering and in an effect and make the register operation idempotent (or register conditionally if it hasn't been registered yet). This will allow the use of the unregister function in the effect cleanup.
That could work well only if tabs always have explicit value props. But in our case, we also support implicit values based on render order, like this:
const [tab, setTab] = React.useState(1);
const handleChange = (event, newValue) => {
setTab(newValue);
};
return (
<Tabs value={tab} onChange={handleChange}>
<Tab label="one" />
<Tooltip title="two helper">
<Tab label="two" />
</Tooltip>
</Tabs>
);Here, tabs derive their value from their render position (i.e., first tab = 0, second = 1), using an internal childIndexRef.
If we register both during render and in an effect:
- In React Strict Mode (dev only), render and effect each run twice i.e total of 4 registrations.
- Even without Strict Mode, a single tab would be registered twice. (one in first render and second in effect).
So, there would be 4 child indexes.
While registerTab is already idempotent when used with explicit values (via valueToIndex.has(finalValue)), we can't enforce value on Tab without introducing a breaking change.
There was a problem hiding this comment.
Deriving the value from the position sounds good to me 👍🏼
While registerTab is already idempotent when used with explicit values (via valueToIndex.has(finalValue)), we can't enforce value on Tab without introducing a breaking change.
But if we create a value for the position on our side, then we would have a finalValue, no?
There was a problem hiding this comment.
Deriving the value from the position sounds good to me 👍🏼
This logic was already present when cloneElement was used. Just picked that up here.
But if we create a
valuefor the position on our side, then we would have afinalValue, no?
Yes, we do generate a finalValue based on position internally when value isn't provided. The issue is that this implicit value depends on render order and a shared index (childIndexRef), which is incremented during registration.
If we allow registerTab to run multiple times — as suggested above, in both render and effect — the index keeps increasing, and the same tab ends up with different finalValues across renders. That breaks selection and causes hydration mismatches.
With explicit value, we don’t rely on index state, so idempotency is safe. But for implicit values, the act of generating finalValue is tied to mutable state — so calling registerTab multiple times isn’t safe unless we move to require explicit values, which would be a breaking change.
There was a problem hiding this comment.
Thanks for working on this @ZeeshanTamboli, it looks to me like a clear improvement on the implementation.
I added some initial questions.
When we release this, we should do it under a minor version just to be cautious about changes that might be perceived as breaking.
Did you add test for all the issues this would close?
| label, | ||
| onChange, |
There was a problem hiding this comment.
I worry this could be perceived as breaking. Could we keep it?
There was a problem hiding this comment.
Nice catch! Updated in 225cafd.
Agreed 👍
Not for every case individually, but I believe this test covers the behavior broadly across the scenarios. Let me know if not. |
There was a problem hiding this comment.
@ZeeshanTamboli I think this change (225cafd) needs to be reverted.
In previous implementation, when onChange is added to both Tabs and Tab, onChange of Tabs always overrides onChange of Tab, check here for implementation =>
In this version, onChange of Tabs doesn't override onChange of Tab thus resulting in broken behaviour of Tabs when both Tabs and Tab has onChange handler.
Check this example built on top of this PR: https://stackblitz.com/edit/ry4fan5c?file=src%2FDemo.tsx, try to change tabs it doesn't work.
I'm aware this is extremely niche use case, not sure if it's really a valid use case, but just wanted to bring it your attention
|
@ZeeshanTamboli I think we should add additional tests for:
(The one we already have covers #46265) |
This reverts commit 225cafd.
👍 Reverted. I think there is no breaking change then cc @DiegoAndai |
|
@ZeeshanTamboli I agree with @sai6855 that |
@DiegoAndai I think we can safely remove the In both the current version (master) and this PR:
You can see this behavior is consistent in both versions: |
@DiegoAndai Addressed in f47c38e |
| @@ -375,7 +376,7 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) { | |||
| scrollbarWidth: 0, | |||
| }); | |||
|
|
|||
| const valueToIndex = React.useRef(new Map()).current; | |||
| const valueToIndex = useLazyRef(() => new Map()).current; | |||
There was a problem hiding this comment.
() => new Map() can be extracted to a function in the module scope so a new one isn't instantiated every time this code runs.
Replace cloneElement with React's Context API without any breaking changes. This closes the following issues:
Tabnot working: Closes [Tabs] Conditionally rendering ofTabnot working properly #34740