fix(theme): keep flat font properties in a mixed configureFonts config - #5066
fix(theme): keep flat font properties in a mixed configureFonts config#5066giaBaoJS wants to merge 1 commit into
Conversation
MikitasK
left a comment
There was a problem hiding this comment.
LGTM 👍
just one follow-up suggestion from my side:
| export default function configureFonts(params: { | ||
| config: Partial<TypescaleStyle> & | ||
| Partial<Record<TypescaleKey, Partial<TypescaleStyle>>>; | ||
| }): Typescale; | ||
| // eslint-disable-next-line no-redeclare |
There was a problem hiding this comment.
wdyt about supporting custom variants here too?
custom variants are documented, but { fontFamily, customVariant: {...} } still produces TS error because this overload only accepts TypescaleKey
maybe we can use a generic mapped overload that treats TypescaleStyle keys as shared properties & other object keys as variants?
it might look smth like that:
| export default function configureFonts(params: { | |
| config: Partial<TypescaleStyle> & | |
| Partial<Record<TypescaleKey, Partial<TypescaleStyle>>>; | |
| }): Typescale; | |
| // eslint-disable-next-line no-redeclare | |
| type MixedFontsConfig<T extends Record<string, unknown>> = T & { | |
| [K in keyof T]: K extends keyof TypescaleStyle | |
| ? TypescaleStyle[K] | |
| : K extends TypescaleKey | |
| ? Partial<TypescaleStyle> | |
| : TypescaleStyle; | |
| }; | |
| type CustomFontVariants<T extends Record<string, unknown>> = { | |
| [K in Exclude< | |
| keyof T, | |
| keyof TypescaleStyle | TypescaleKey | |
| >]: TypescaleStyle; | |
| }; | |
| export default function configureFonts< | |
| T extends Record<string, unknown>, | |
| >(params: { | |
| config: MixedFontsConfig<T>; | |
| }): Typescale & CustomFontVariants<T>; | |
| // eslint-disable-next-line no-redeclare |
btw, this could be handled separately in follow-up PR
There was a problem hiding this comment.
Agreed, and I would rather keep this PR at its current size given it is already approved. I will open the follow-up once this lands.
Two things I checked in the meantime.
The gap is types-only. configureFontsConfig already builds custom variants at runtime: variantNames unions Object.keys(typescale) with Object.keys(variantOverrides) (src/theme/fonts.tsx:35-38), so { fontFamily, customVariant: {...} } produces the correct object today and it is only the overloads that reject it. Confirmed the error you describe: TS2769: No overload matches this call. The last overload gave the following error. Type 'string' is not assignable to type 'TypescaleStyle'.
Your sketch works. I prototyped it locally: T infers through MixedFontsConfig<T>, configureFonts({ config: { fontFamily, customVariant: {...} } }).customVariant.fontSize comes back as number, and the existing call shapes ({ fontFamily } alone, { fontFamily, bodyLarge }, and no argument) all still compile.
The one thing I want to settle in the follow-up is overload ordering. Placed first, the generic also claims the plain and mixed built-in cases, so their return type becomes Typescale & CustomFontVariants<T> instead of plain Typescale. That is structurally the same thing, but I would rather have a test per config shape before changing which overload wins.
The bug
configureFontsConfigclassifies the config as a whole:A config may legitimately contain both shapes at once — a font property that should apply to every variant, plus a tweak to one variant. A single per-variant object flips
isFlatConfigtofalsefor the whole config, and theObject.assignbranch below then spreads every top-level entry as if it were a variant.Repro:
Two things go wrong:
fontFamilyis silently dropped from every variant —bodyLarge.fontFamilyis still"System".No error, no warning. From the app's point of view the custom font just does not apply.
The same shape is also rejected by the type overloads today, so a TS user gets
TS2769: No overload matches this calland has to callconfigureFontstwice or hand-build the object.The fix
Partition
Object.entries(config)into scalar entries (shared font properties) and object entries (per-variant overrides), instead of classifying the config as a whole. Build every variant as{ ...typescale[variant], ...sharedProperties, ...variantOverrides[variant] }.The two previous branches are special cases of this, so the
isFlatConfigearly return and theObject.assignboth go away:variantOverridesis empty → every variant gets the scalars (old flat branch)sharedPropertiesis empty → named variants are merged, others untouched (old per-variant branch)I also added a third overload for the mixed shape:
It is placed after the two existing
Partialoverloads and before theRecord<string, TypescaleStyle>one. Neither existing overload accepts a mixed literal (each rejects the other's keys as excess properties), and the custom-variant overload does not either, so this signature only picks up calls that previously failed to compile — existing call sites keep resolving to the overload they resolved to before. I verified both directions: the mixed literal isTS2769onmainand clean with this change, while the flat-only and per-variant-only literals compile on both.About #4589
I could not reproduce the reporter's
Variant titleLarge was not provided properlyfrom this bug, and I don't think this closes it — the warning there listsregular, medium, light, thin, i.e. an MD2-shapedfontsobject, which is a different path entirely. Listing it as possibly related only, deliberately notFixes.Test plan
Two cases added to
src/theme/__tests__/fonts.test.js:Before the change both fail; the second reports
Received: {"0": "N", "1": "o", "2": "t", "3": "o", "4": "S", "5": "a", "6": "n", "7": "s"}. The four existingconfigureFontstests pass unchanged, before and after.yarn test: 55 suites, 737 passed / 1 skipped, 169 snapshots — no snapshot churn.yarn lintandyarn typecheckclean.