[types] cursor should accept arbitrary strings - #1880
Closed
henryqdineen wants to merge 5 commits into
Closed
henryqdineen wants to merge 5 commits into
henryqdineen wants to merge 5 commits into
Conversation
`gen-types` translates the Flow types into TypeScript, which loses the most
useful thing these types can do, because a Flow union containing `string`
collapses to `string` and discards every literal member:
Flow: type Display = 'block' | 'flex' | string;
TypeScript: type Display = 'block' | 'flex' | (string & {});
`string & {}` is mutually assignable with `string`, so any string is still
accepted, but the literals survive and editors keep suggesting them. There is
no way to express this in Flow.
`gen-types` copies a `.d.ts` verbatim when one sits beside the Flow source, so
checking this file in is enough to take over that translation -- no build
changes. Verified byte-identical to what `gen-types` produced, once the 249
`string` -> `(string & {})` substitutions are reversed.
100 of the 522 properties were typed as bare `string` or `number | string` and
so offered no completions at all; another ~340 lost their literals to a
`string` member. Those now autocomplete.
`packages/typescript-tests/src/open-unions.ts` covers the behaviour: arbitrary
strings and `var()` are still accepted, `null` still unsets a property, the
CSS-wide keywords still resolve, literals survive, and closed unions stay
closed. Note `NonNullable` cannot be used to inspect these unions -- it is
`T & {}` in TypeScript >=4.9, and that intersection flattens them.
The two files must now be kept in sync by hand. Adding a css-tree-based test to
enforce that, and to check the literals against the CSS grammar, is the
intended follow-up.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five things about `CSSProperties` are hand-maintained and can drift silently.
`StyleXCSSTypes.d.ts` duplicates `StyleXCSSTypes.js` so that TypeScript can use
`(string & {})` where Flow only has `string`. Nothing stops the two files
diverging, so the first test reads both, reduces each property to the keywords,
numbers and strings it accepts, and asserts the two agree. `string & {}` reads
as an arbitrary string, which is exactly what it is.
`@babel/parser` reads both dialects -- the Flow types with the `flow` plugin,
the definitions with `typescript` in declaration mode -- so one reader covers
both files. The two ASTs use disjoint node names, so a single switch handles
either.
The CSS-wide keywords are valid on every property and appear in no grammar, so
the second test is the only thing that can check them. Properties compose them
from the `all` alias.
The property values are written by hand, so `css-tree`'s value definition
grammar checks them in both directions. The third test asserts every keyword in
the types is one the grammar accepts, catching typos and invented values. The
fourth asserts the reverse for closed unions -- that every keyword the grammar
allows is typed -- catching values a user simply cannot write. Both follow
`<'other-property'>` references, which is how logical properties and shorthands
are defined.
The reverse check is restricted to closed unions on purpose. Where a property
accepts an arbitrary string, every keyword is already accepted and completeness
means nothing; asserting it everywhere demands over ten thousand additions,
because shorthands like `background` transitively pull in every named colour.
Restricted, it finds 125 real omissions across 33 properties.
The set of properties is also hand-maintained, so the fifth test asserts every
Baseline-available property is typed. `web-features` is the data behind MDN's
"Widely available" / "Newly available" banners; css-tree carries no support
data at all, and mdn-data's `status` describes the spec rather than browsers --
it calls `font-stretch` obsolete though every browser has supported it since
2020, while marking its replacement `font-width` experimental. Using Baseline
rather than "css-tree knows about it" keeps the exclusion list to real
decisions: 25 entries rather than 82.
All five pass as-is. Each list entry carries a reason, and each failure says
what to do about it:
- 6 properties accepting none of the CSS-wide keywords, written
`null | 'a' | 'b'` rather than composing `all`. They are also the only
properties in the file with no named type alias -- inlining the union at the
property is what hid the slip.
- 17 properties css-tree has no grammar for: abandoned drafts (`motion-*` became
`offset-*`, the CSS Display Level 3 longhands were dropped), `@font-face`
descriptors that were never properties (`src`, `unicode-range`), and names
that never shipped unprefixed.
- 18 properties keeping a keyword the grammar has dropped, where real
stylesheets still use it. `marginTrim` is the clearest: the spec was rewritten
to `none | in-flow | all`, but Safari ships the original `block`/`inline`
syntax.
- 33 properties whose closed union is missing keywords the grammar allows --
`display` cannot take `flow` or `table-caption`, `overflow` cannot take
`overlay`, `mixBlendMode` cannot take `plus-darker`. `marginTrim` appears in
both keyword lists, which is honest: it keeps values the spec dropped and is
missing values the spec added.
- 25 Baseline-available properties that are not typed: 7 that should be, 17 SVG
presentation attributes, and `all`.
`web-features` is pinned exactly rather than by range, because its whole
purpose is to change over time: a floating version would let a routine install
flip a property to Baseline and fail the build with no code change. Pinned, the
test only fires on a deliberate bump -- which is when the news is wanted.
Verified by injecting each failure: a typo'd keyword trips the grammar and
parity tests and is reported by name; editing one type file without the other
trips only parity; removing an allowlist entry reports what to add, and
satisfying one reports the entry as stale.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Worked example of the CSS-wide keyword loop: add the two missing keywords to
`GLOBAL_KEYWORDS`, and the test names every property that cannot take them.
display: does not accept 'revert', 'revert-layer'. Either compose the
`all` alias, or add it to NO_GLOBAL_KEYWORDS with a reason.
`revert` and `revert-layer` are CSS-wide keywords, valid on every property, but
the `all` alias that all 516 properties compose stopped at `unset`. `revert`
has been Baseline since 2021 and `revert-layer` since 2022; csstype has carried
both for years.
Fixed in one place, since `all` is the single definition. Pure widening -- no
value that used to typecheck stops doing so.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Worked example of emptying an allowlist: delete `NO_GLOBAL_KEYWORDS` and the
test names each property and what it is missing.
colorScheme: does not accept 'inherit', 'initial', 'unset', 'revert',
'revert-layer'. Every property should compose the `all` alias.
These six were written `null | 'a' | 'b'` rather than `all | 'a' | 'b'`, so
they accepted none of the CSS-wide keywords -- `colorScheme: 'inherit'` was a
type error, as were `marginTrim: 'unset'`, `paintOrder`, `textJustify`,
`WebkitBackgroundClip` and `WebkitBoxOrient`.
They are also the only six properties in the file with no named type alias:
everything else declares `type foo = ...` and writes `foo?: all | foo`, while
these inline their union at the property. Reaching for `null` -- what `all`
happens to start with -- instead of composing `all` is the slip that inlining
hid. Five date to the initial commit; `WebkitBackgroundClip` was added later by
"Chore: Allow additional -webkit- properties (facebook#28)" and copied the shape.
Pure widening. The allowlist is now gone rather than shortened, so nothing can
reintroduce the pattern.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Worked example of the `CLOSED_DESPITE_GRAMMAR` loop, and it closes facebook#1463. cursor: its grammar is `[ [ <url> [ <x> <y> ]? , ]* [ auto | default | ... ]]`, which admits more than keywords, but the type accepts no arbitrary string. Either add `string` in the Flow types and `(string & {})` in the definitions, or list it in CLOSED_DESPITE_GRAMMAR with a reason. `cursor` takes a comma-separated list of `url()` values before its keyword, so a union of literals can never describe it -- `cursor: 'url(cur.png) 4 12, auto'` was a type error. This is the change proposed in facebook#1466, except the TypeScript definitions get `(string & {})` rather than a bare `string`, so all 43 cursor keywords keep autocompleting while any string is accepted. That was the thing that PR asked for and could not have while the definitions were generated from the Flow types; facebook#1466 can be closed in favour of this. Opening the union also made its `INCOMPLETE_UNIONS` entry unreachable, since that check skips properties accepting arbitrary strings. Rather than leave a dead entry, the seven keywords it recorded are added: `hand` (the pre-standard IE spelling) and the `-moz-` and zoom variants, where StyleX had `-webkit-grab` and `-webkit-grabbing` but not their siblings. Confirmed after the change: `url(cur.png) 4 12, auto`, `-moz-grab` and `pointer` all typecheck, and the editor still offers 43 completions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@henryqdineen is attempting to deploy a commit to the Meta Open Source Team on Vercel. A member of the Team first needs to authorize it. |
This was referenced Sep 11, 2026
meta-cla
Bot
added
the
CLA Signed
Collaborator
Author
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.
What changed / motivation ?
cursortakes a comma-separated list ofurl()values before its keyword:A union of literals can never describe that, so
cursor: 'url(cur.png) 4 12, auto'was a type error — the bug reported in #1463.This obsoletes #1466. That PR fixes the same issue by adding a bare
stringtoCSSCursor. Here the TypeScript definitions get(string & {})instead, so all 43 cursor keywords keep autocompleting while any string is accepted — which is what I wanted in #1466 and could not have while the definitions were generated from the Flow types. #1466 can be closed in favour of this.Opening the union also made
cursor'sINCOMPLETE_UNIONSentry unreachable, since that check skips properties accepting arbitrary strings. Rather than leave a dead entry, the seven keywords it recorded are added:hand(the pre-standard IE spelling) and the-moz-and zoom variants, where StyleX had-webkit-graband-webkit-grabbingbut not their siblings.Linked PR/Issues
Fixes #1463. Obsoletes #1466. Stacked on #1879.
Additional Context
Found mechanically rather than by inspection: test 5 from #1878 asserts that a property whose grammar admits non-keyword values accepts arbitrary strings, and reported this along with its grammar and the exact two-file edit.
Confirmed after the change:
cursor: 'url(cur.png) 4 12, auto',cursor: '-moz-grab'andcursor: 'pointer'all typecheck, and the TypeScript language service still offers 43 completions forcursor.Pre-flight checklist
Contribution Guidelines
🤖 Generated with Claude Code