Review/refactor batch - #2274
Merged
Merged
Conversation
React calls getSnapshot on every render, and the previous implementation built a fresh MediaQueryList through window.matchMedia on each of those calls. Measured on a single hook instance: 4 matchMedia calls on mount and 2 more on every re-render (doubled again under StrictMode). Caching the MediaQueryList in a ref, keyed by the normalized query, brings that down to 1 call on mount and 0 per re-render. The list is still resolved lazily inside subscribe/getSnapshot, so the hook remains server-safe, and the cache is per hook instance, so it never outlives the component or leaks between environments. Also drops the duplicate normalize() pass that ran on every subscribe and every snapshot. Costs 33 B brotlied, so the size budget moves from 145 B to 180 B.
breakpoints.keys was typed as readonly string[], so it could not be fed back into the API it belongs to: theme.breakpoints.keys.map((k) => theme.breakpoints.up(k)) // TS2345: 'string' is not assignable to '"xs" | "sm" | ... | "xxl"' Narrowing it to readonly (keyof T & string)[] makes iterating over the breakpoints type-check without a cast, and the narrowed type reaches consumers through the published declaration file. buildBreakpointsMap now carries the key type through sort/map instead of letting Object.fromEntries widen everything to any, so the one remaining assertion is the unavoidable Object.entries one.
The wrapper built its methods by looping over Object.keys(validators) and spreading Object.fromEntries(...) into the theme. That spread is typed any, so the declared StyledBreakpointsTheme<T> return type was never enforced: replacing a method body with `return 42` compiled cleanly. The validators themselves took (...args: any[]), so nothing downstream was checked either. Each guarded method is now built explicitly, with its argument tuple inferred from the method it wraps, and memoize carries that tuple through instead of collapsing to any[]. The same mistake now fails with TS2322, and the file contains no `any`. Validator params became string | undefined, which is what they always received at runtime for omitted arguments — the arity check already covers that case, so every error message is byte-for-byte unchanged. Also drops the buildBreakpointValidators export, which had no callers outside this module. Costs ~20 B brotlied: the dev bundle goes 1.52 kB -> 1.54 kB against a 1.55 kB budget, so headroom is now 10 B.
The orientation union was spelled out inline in 10 places and the
`Record<string, \`${number}px\`>` constraint in 3, even though a Values alias
already existed. The type test had to declare its own Orientation because the
library never exported one.
- adds an exported Orientation alias, used everywhere the union appeared
- uses the existing Values alias for both generic constraints
- derives the up/down/between/only implementations from ThemeBreakpoints<T>
instead of respelling each parameter list in create-theme
- gives withValidation an explicit StyledBreakpointsTheme<T> return type
- re-exports Config, Orientation, StyledBreakpointsTheme, ThemeBreakpoints
and Values from the public entry
Occurrences drop from 10 to 1 and from 3 to 1. The generated declaration file
now returns the named StyledBreakpointsTheme<T> instead of an inlined shape,
and all 47 type-level assertions still pass unchanged, so the public type
contract is identical.
validateConfig threw inside its own try block purely so the catch could
re-throw the same message with the error prefix attached. That required
silencing the preserve-caught-error lint rule, and the catch-all swallowed
genuine failures too — an unexpected TypeError would have been reported to
users as "Theme configuration failed".
The checks now return the issue as a string and validateConfig throws once,
at the top. No try/catch, no eslint-disable, and an unexpected error
propagates with its own stack instead of being relabelled.
All eight configuration error snapshots are unchanged. The slightly
misleading wording for { breakpoints: {} } — reported as "breakpoints" must
be defined even though the key is present but empty — is left as is, since
changing user-facing error text is a separate decision.
toQuoted was exported but had no callers outside formatters.ts, where it is only reached through toQuotedList. Dropping the export makes the module's surface match what it actually offers. No size change — tree-shaking already kept the function.
Reverts the parts of the previous commit that put named aliases into the generated declaration file. Orientation, the Values constraint on the public entry, and an explicit StyledBreakpointsTheme<T> return type on withValidation all made hovers and signature help show a bare alias name, so a caller could no longer see which orientation values are accepted or what the factory returns without chasing definitions. What stays from that commit is the part that is invisible to consumers: create-theme derives up/down/between/only from ThemeBreakpoints<T> rather than respelling every parameter list, and uses the Values alias for its internal generic constraint. types.ts and with-validation.ts now carry comments explaining why the inline union and the inline constraint must not be factored out. Verified: dist/dev/index.d.ts is byte-identical to master.
The caching added in the previous commit had no permanent coverage — it was verified with a throwaway probe that counted window.matchMedia calls and then deleted. Nothing stopped a later change from moving the matchMedia call back into getSnapshot. Two tests now pin both directions: - re-rendering must not resolve another MediaQueryList. Against the previous implementation this fails with 8 calls growing to 16 over two re-renders (StrictMode doubles React's getSnapshot calls). - changing the query must resolve a new one, so the cache cannot go stale.
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2274 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 10 10
Lines 163 163
Branches 27 29 +2
=========================================
Hits 163 163 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
🎉 This PR is included in version 15.0.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.