feat: strict cssTypes - #123305
Conversation
github-actions
Bot
added
the
Scope: Frontend
📊 Type Coverage Diff
🔍 1 new type safety issue introducedType assertions (
This is informational only and does not block the PR. |
| height="100%" | ||
| position="relative" | ||
| minHeight={typeof minHeight === 'number' ? `${minHeight}px` : minHeight} | ||
| minHeight={minHeight} |
There was a problem hiding this comment.
runtime check is not necessary since our csstypes only allow strings except for 0 (which can be unitless)
There was a problem hiding this comment.
not claiming that this is complete, but I let AI come up with all the units and made sure it passed all our current usages. We can amend this as we see fit in the future.
There was a problem hiding this comment.
I think this is an OK start but I'd love if we aligned more with the values and units spec for the internal types.
| @@ -0,0 +1,57 @@ | |||
| type Globals = 'inherit' | 'initial' | 'revert' | 'revert-layer' | 'unset' | CssFunction; | |||
| type CssFunction = `${string}(${string})`; | |||
There was a problem hiding this comment.
note:CssFunction is purposefully quite wide. It covers:
var(--something)
calc(something)
min(something, something)
minmax(...)
etc. We can narrow this if we want. One problem is that we can’t do:
`var(--${string})` | `${string}(${string})`
because then something like var(++foo) would still work because the second part of the union is wider than the first. So if we make this more narrow, we need an explicit list, which is why I didn’t do this in the first iteration.
| <Container position="relative" className={className}> | ||
| <RightPaddedTextArea autosize rows={5} maxRows={10} {...props} /> | ||
| <Container position="absolute" top="8px " right="10px"> | ||
| <Container position="absolute" top="8px" right="10px"> |
There was a problem hiding this comment.
wat !? I think the extra blank doesn’t matter at runtime but still ...
| {Object.entries(props.tokens).map(([token, value]) => ( | ||
| <Token key={token} scale={props.scale} {...{token, value}}> | ||
| {props.renderToken({value, token})} | ||
| {props.renderToken({value: value as T[keyof T], token})} |
There was a problem hiding this comment.
Object.entries sadly widens the type of value to string
| <Flex height={CHART_HEIGHT} justify="center" align="center"> | ||
| <Flex justify="center" align="center"> | ||
| <Placeholder height={`${CHART_HEIGHT}px`} /> | ||
| </Flex> |
There was a problem hiding this comment.
this was a real bug. height needs a unit, but CHART_HEIGHT doesn’t have one because we pass it to echarts, which expects a number. Looks like this in the browser:
now this didn’t have any impact, because the Placeholder has the correct height, which is why I removed it. But it now type-errored which is nice
Story previewsPreview the stories changed in this PR on the Vercel deployment:
Preview deployment: https://sentry-ggejdn6pj.sentry.dev |
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit e107f8a. Configure here.
natemoo-re
left a comment
natemoo-re
left a comment
There was a problem hiding this comment.
Very nice! Few suggestions on the module shape but overall really happy with this change and the bug catches!
There was a problem hiding this comment.
I think this is an OK start but I'd love if we aligned more with the values and units spec for the internal types.
| */ | ||
| type BaseUnit = 'px' | 'em' | 'rem' | 'ex' | 'rex' | 'cap' | 'rcap' | 'ch' | 'rch' | '%'; | ||
|
|
||
| type WidthUnit = |
There was a problem hiding this comment.
Technically there's no delineation between width/height units in the spec. Both are valid <length> units for any property. Was it intentional to narrow the available surface?
There was a problem hiding this comment.
yes it was intentional. do we ever need to set the height to something like “viewport width” ?
| export type CssWidth = BaseSize | `${number}${WidthUnit}` | Globals; | ||
| export type CssMinWidth = CssWidth; | ||
| export type CssMaxWidth = CssWidth | 'none'; | ||
| export type CssHeight = BaseSize | `${number}${HeightUnit}` | Globals; | ||
| export type CssMinHeight = CssHeight; | ||
| export type CssMaxHeight = CssHeight | 'none'; | ||
| export type CssInset = `${number}${HeightUnit}` | Globals | BaseDimension; |
There was a problem hiding this comment.
Thoughts on structuring this as CSS['width'], CSS['minWidth'], etc? IMO that feels like the more familiar and discoverable pattern.
3 participants
This PR adds stricter cssTypes implementations than
React.CSSProperties, which has no type guarantees as everything is unioned with(string & {}), making it a type-hint at best.In doing so, I found a couple of bugs where we pass wrong things (like unit-less numbers to width/height) that lead to ignored css styles.