fix(provider): deep merge theme prop so a partial theme keeps defaults - #5047
fix(provider): deep merge theme prop so a partial theme keeps defaults#5047giaBaoJS wants to merge 2 commits into
Conversation
`PaperProvider` spread `props.theme` over the base theme and only
re-merged `colors`. Every other default sub-object — `fonts`, `shapes`,
`motion`, `elevation` — was replaced wholesale, so supplying a partial
`theme.fonts` wiped all 15 MD3 typescale variants and made every
`<Text variant="...">` throw:
Variant titleLarge was not provided properly.
Valid variants are regular, medium, light, thin.
Merge with `safeMerge` instead — the helper `useInternalTheme` already
uses for the per-component `theme` prop — so provider-level and
component-level themes merge with the same semantics. `animation.scale`
is still resolved last so reduce-motion keeps overriding it.
Fixes callstack#4589
| // Deep merge so a partial theme extends the defaults instead of replacing | ||
| // them. `safeMerge` is the same helper `useInternalTheme` uses for the | ||
| // per-component `theme` prop, which keeps both levels consistent. | ||
| const merged = safeMerge(base, props.theme); |
There was a problem hiding this comment.
calling safeMerge at the theme root can drop all defaults when a custom theme contains dynamic, semantic or resource_paths
current sentinel check treats any object containing one of these keys as a native color, while custom theme properties are supported
safeMerge(base, { dynamic: true });
// => { dynamic: true }, dropping fonts, colors, shapes etc.
RN’s actual native-color values have narrower shapes, such as { semantic: string[] } or { dynamic: { light, dark } } (source)
so could we validate those shapes before using safeMerge here & add regression test for custom dynamic property?
There was a problem hiding this comment.
Good catch — you're right, and this is a real regression introduced by moving safeMerge to the theme root. Fixed in 25363ae.
Reproduced first: safeMerge(base, { dynamic: true }) returned { dynamic: true }, and <PaperProvider theme={{ dynamic: true }}> made <Text variant="titleLarge"> throw again — the exact symptom this PR set out to fix.
isPlatformColorSentinel now validates the value, not just the key name. Checking the shapes React Native actually emits (PlatformColorValueTypes.ios.js L25-28 and L37-50, PlatformColorValueTypes.android.js L19-23), a native color value carries exactly one key and nothing else, so the check now accepts only:
{ semantic: string[] }— iOSPlatformColor{ resource_paths: string[] }— AndroidPlatformColor{ dynamic: { light, dark, highContrastLight?, highContrastDark? } }—DynamicColorIOS, requiring bothlightanddarkand rejecting any key outside that tuple
Everything else — including { dynamic: true }, { semantic: 'label' }, and any object that carries a sentinel key alongside siblings such as colors/fonts — is now an ordinary object and gets deep-merged, so a documented custom theme property called dynamic keeps every default.
Regression tests added, as requested:
PaperProvider.test.tsx: a theme{ dynamic: true, colors: { primary: 'tomato' } }keeps the typescale, palette and shape tokens, plus a user-visible one asserting<Text variant="titleLarge">renders undertheme={{ dynamic: true }}.provider.test.ts: rejection cases for the three key names with wrong-shaped values and with sibling keys.
Two of the new tests are deliberately non-vacuous controls that pass both before and after, so the tightening can't quietly degenerate into "nothing is ever a sentinel": one asserts real PlatformColor('label') and DynamicColorIOS(...) values are still detected, and one asserts a real DynamicColorIOS override still passes through safeMerge by identity, with no highContrast* keys inherited from the base sentinel underneath it. With the sentinel change reverted the six new tests fail and those two still pass.
Suite: 55 suites / 737 → 745 passed / 1 skipped, 169 snapshots unchanged; lint and typecheck clean.
`isPlatformColorSentinel` matched on key name alone, so any object owning
a `dynamic`, `semantic` or `resource_paths` key was treated as an opaque
native color and returned from `safeMerge` untouched. Extending the theme
with custom properties is documented, so a theme like
{ dynamic: true, colors: { primary: 'tomato' } }
was mistaken for a native color at the theme root and replaced the base
theme wholesale — dropping fonts, shapes, motion and elevation. Harmless
before this branch, where `safeMerge` never saw the theme root, but a
real regression now that `PaperProvider` merges there.
Validate the value against the shapes React Native actually emits in
`PlatformColorValueTypes.{ios,android}.js`: exactly one of the three keys
and nothing else, `semantic`/`resource_paths` holding an array of strings,
`dynamic` holding a tuple with both `light` and `dark` and no key outside
{light, dark, highContrastLight, highContrastDark}.
Values from the real `PlatformColor()` and `DynamicColorIOS()` are still
detected and still merged as leaves; that is asserted directly against
the react-native APIs so the tightening cannot degenerate into never
matching.
Summary
PaperProviderreplaces the whole theme object instead of merging it, so any partialthemeprop silently drops the defaults it doesn't mention. The most visible casualty isfonts: supplying one makes every<Text variant="…">in the app throw.Fixes #4589.
Reproduction
@lukewalczak asked for a repro on the issue and never got one, so here it is as a runnable snippet — this is the whole thing, no app scaffolding needed:
Throws immediately:
That trailing list is literally
Object.keys(theme.fonts)inText.tsx#L136-L138printing back the user's four keys — proof that the 15 MD3 typescale variants are gone by the timeTextreads the theme, not that the user'sfontswas malformed. The in-thread workaround (pinning to 5.12.3) matches: this predates the typescale becoming mandatory.It is committed as a regression test in this PR, so the repro lives in CI rather than in a Snack that can rot.
Root cause
src/core/PaperProvider.tsx#L39-L44:colorsgets re-merged againstbaseon the line below the spread. Nothing else does....props.themetherefore replacesfonts,shapes,motionandelevationwholesale with whatever fragment the user passed.Worth stressing that this is an internal inconsistency, not just a missing case.
src/theme/provider.tsxalready exportssafeMerge, anduseInternalThemeuses it to deep-merge the per-componentthemeprop. So today:The same prop, on the same shape, with two different merge semantics depending on where you put it.
fontsis what #4589 reports because it is the only sub-object with a hardthrowbehind it.shapes,motionandelevationfail identically but silently — and they're nested two or three levels deep (shapes.corner.small,motion.spring.fast.spatial.stiffness), so a one-level merge wouldn't save them either.The fix
Reusing
safeMergerather than adding afonts:line next to thecolors:one, because:shapes/motion/elevationtoo, which have the identical defect and are too deeply nested for a shallow merge;themeprops agree, which is the actual inconsistency here;PlatformColorsentinel handling added in8720eac— no new merge logic enters the codebase;animation.scaleis still applied last soreduceMotionkeeps overriding it.I'm happy to switch to the minimal,
fonts-only version if you'd rather keep the blast radius at exactly the reported bug:colors: { ...base.colors, ...props.theme?.colors }, + fonts: { ...base.fonts, ...props.theme?.fonts },Just say the word and I'll swap it — but it leaves the other three broken.
Behaviour change to be aware of
Anyone deliberately replacing rather than extending a sub-object now gets the defaults merged underneath it. In practice this is hard to observe: a complete
configureFonts()output overrides every key anyway (covered by a test below), and there's no way to express "and delete the rest" through a$DeepPartialprop today. But it is a real semantic change and worth a line in the release notes.Deep merge also means individual variants inherit missing properties —
fonts: { titleLarge: { fontSize: 20 } }now keeps the defaultlineHeight/letterSpacinginstead of dropping them. That's the behaviouruseInternalThemealready has, and it's the friendlier of the two.Tests
Five tests added to
src/core/__tests__/PaperProvider.test.tsx, using the existingFakeChild+useThemeharness. Three fail onmain, two pass onmainas controls.Fail before the fix:
keeps the base typescale when only part of theme.fonts is provided—theme.fonts.titleLargeisundefinedonmain.renders <Text variant> instead of throwing when theme.fonts is partial— asserts the user-visible symptom. Onmainit fails with the issue's error string verbatim, which is the point: the shape assertion above could pass while the app still crashed.keeps sibling tokens when a nested shape token is overridden— demonstrates the non-fontshalf of the bug.Pass before and after, as non-vacuity controls:
still merges theme.colors with the base palette—colorswas already correct; this proves the harness observes real merging rather than always going green.lets a complete fonts object override every default variant— guards the behaviour change above: a full typescale still wins outright.I deliberately did not add a test for the
animationline....props.theme?.animationdropsbase.animationfor the same reason, butTheme['animation']currently has exactly one key (scale) which is recomputed on the next line, so the omission is unobservable and any test for it would pass with or without the change. The fix restores...merged.animationanyway so it stays correct if a second key is ever added.Verification
Zero snapshot churn — all 169 pass unwritten, since they render under the default theme where deep and shallow merge coincide.
yarn lintandyarn typecheckare clean.