fix: six small findings, plus a windows CI lane - #428
Open
YevheniiKotyrlo wants to merge 6 commits into
Open
Conversation
`colorScheme.set()` called `Appearance.setColorScheme`, which does not exist.
The web plane imports `Appearance` from "react-native" and the bundler
substitutes react-native-web, whose Appearance reads through to
`matchMedia("(prefers-color-scheme: dark)")` and exposes only `getColorScheme`
and `addChangeListener`. Every call was a TypeError.
TypeScript cannot see this: it resolves react-native's `.d.ts` for that import
either way, so the substitution is invisible to `yarn typecheck`.
The browser owns the color scheme on web and evaluates
`@media (prefers-color-scheme)` itself, so there is no observable for an
override to drive the way there is on native. Throwing names that constraint;
returning silently would leave an in-app theme toggle broken with nothing to
find.
The test runs the web module against react-native-web, which is the first
coverage the web plane has had.
The compiler had no `@media (prefers-color-scheme: ...)` case. The one assertion of that condition reaches it through `light-dark()`, where the compiler synthesises the condition, so the parse path was unproven. The runtime half had no direct coverage either — `testMediaQuery` was only ever reached through a rendered component, which also exercises the collection and the resolver, so a condition evaluated wrongly could still produce the right style. The platform block was `describe.skip`. It is stale expectations, not a gap: the conditions it asserts are produced correctly today, and the block fails only on drift the compiler has since accumulated — `#ff0000` now shortens to `#f00`, specificity is `[2, 1]`, and a `v` entry carries the inherited color. The ios case also asserted one nesting level too few on `m`, which is an authoring slip rather than drift. Corrected and unskipped.
`withReactNativeCSS` installs the `resolveRequest` that decides whether the native and web resolvers run at all, and nothing exercised it. The resolvers themselves are covered; the dispatch into them was not, so the gate could be inverted, defaulted the other way, or dropped without a test noticing. Covers both settings on both platforms, the metro-override short-circuit, and the preference for a config's existing `resolveRequest` over the context's.
`inlineVariables` inlines a custom property that has exactly one declaration,
so `:root { --my-var: red }` compiles to a literal with no root variable entry
at all. A test written that way asserts the inliner and keeps passing with the
runtime variable registry deleted, which makes it silently worthless. Use count
does not save it: the pass counts declarations, so one declaration read from
ten rules is still inlined.
Rendering cannot tell the two apart either, because the inlined literal and the
resolved variable produce the same style. Only the compiled stylesheet shows
the difference.
`dynamicRootVariables` emits a second declaration behind a guard that never
matches, so the property stays dynamic. Both declarations carry the same value,
so the resolved value does not depend on the guard staying unmatched.
The tests pin the inliner's behaviour as well as the helper, so if it stops
inlining or starts keying on use count the helper can be retired.
Every job runs on ubuntu except one macos builder, so nothing in CI executes
this codebase on Windows. The babel plugin, the metro resolver and the compiler
all join and compare file paths, which is exactly the class of code a POSIX-only
matrix cannot vet.
That gap is not theoretical. At this commit, on a Windows host, three of the
repo's own tests fail: "7. import View from '../View/View'" in
src/__tests__/babel/react-native.test.ts, and "6. import View from '../View'"
and "17. const View = _interopRequireDefault(require('../View'))" in
src/__tests__/babel/react-native-web.test.ts. Relative imports are not
rewritten because the separator comparison assumes forward slashes.
Coverage stays on the ubuntu job; this one only needs to be able to fail.
Ordering: the fix for those three lives on fix/babel-windows-posix-paths. Merge
that branch first, or this job lands red.
… is set `Appearance.getColorScheme()` answers null whenever the OS reports `unspecified` or the native module is absent, so a null scheme is a reachable production state rather than a test-harness artifact. Comparing the queried value straight against it made `@media (prefers-color-scheme: light)` match nothing in that state, so an explicit light rule silently never applied. MQ5 resolves the absence of a preference to `light`, and the rest of the library already assumes that. `light-dark(red, blue)` compiles to a light base rule plus the dark value behind `["=", "prefers-color-scheme", "dark"]`; `colorScheme.get()` on native ends in `?? "light"`; and react-native-web's `getColorScheme()` reads the dark media query and answers "light" when it does not match. Only this comparison disagreed, so a light rule behaved differently on the two platforms. Resolving the scheme before the comparison rather than branching on it keeps an unrecognised value false, which MQ5 also requires.
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.
Six small independent findings, each separable — split any of them out if you would rather take them apart.
Fixes
prefers-color-schemeresolved to nothing when the OS reported no preference. It now resolves tolight, which is what the CSS spec saysno-preferencemeans for this feature. Without it adark:rule and its unprefixed counterpart could both fail to apply.src/web/api.tsxcalled a method that does not exist.react-native-web@0.21.1'sAppearanceexports onlygetColorSchemeandaddChangeListener— there is nosetColorScheme, socolorScheme.set(…)was a hardTypeErroron web. It is invisible totscbecause the file importsAppearancefrom"react-native", so TypeScript resolves React Native's own.d.tswhile the bundler substitutesreact-native-web. It now reports that the scheme cannot be set on web rather than throwing.Tests, and one CI lane
ci: run the unit suite on windows.ci.ymlwasubuntu-latestplus onemacos-15, with no Windows runner — which is why a three-test failure sat inmainunnoticed and why a previous cross-platform test of mine was inert on the only runner that existed. This is the smallest change here and probably the most valuable.prefers-color-schemehad no compiler test.media-query.test.tshad no case for it — its platform block isdescribe.skip, leaving onlyhover. The single existing assertion of that condition reached it vialight-dark(), where the compiler synthesises the condition, so the@mediaparse path was unproven. The platform media queries are revived in the same commit.src/metro/had no tests at all.metro/resolver.tsis the only import rewriter whenglobalClassNamePolyfillis false, and the babel plugin the only one when it is true — they are alternatives, not layers, so one of the two rewriting paths was entirely unexercised. This covers the gate.dynamicRootVariablesso a:roottest reaches the runtime rather than asserting only the compiled output.Note
Item 2 is a behaviour change on web for anyone currently calling
colorScheme.setthere — today that call throws, so nothing can be depending on it working, but it is the one item here that is not purely additive. Happy to split it out if you would rather take it separately.yarn typecheckandyarn lintexit 0. The 3 remaining failures are the known Windowsbabel-plugin-testerbaseline, present onmain.