fix: six small findings, plus a windows CI lane - #428
Open
YevheniiKotyrlo wants to merge 6 commits into
Open
YevheniiKotyrlo wants to merge 6 commits into
YevheniiKotyrlo wants to merge 6 commits into
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.
Contributor
Author
Why no device screenshotFive of the six findings are test and CI coverage, and the sixth changes which rule applies rather than how a frame is painted. The one worth measuring is the web The three are the Windows-only |
This branch has not been deployed
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.
Problem
Six small independent findings, each separable — split any of them out if you would rather take them apart.
prefers-color-schemeresolved to nothing when the OS reported no preference. MQ5 §12.5 gives the featurelight | darkand no third value — unlikeprefers-reduced-motion(§12.1), which does haveno-preference— so resolving to nothing puts it in a state the feature has no value for, and(prefers-color-scheme: dark)and(prefers-color-scheme: light)are false at once. It now resolves tolight.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.Solution
Six small findings, each independent.
Web reports that the colour scheme cannot be set there rather than failing quietly, and
prefers-color-schemeresolves tolightwhen the host states no preference. Which of the two MQ5 §12.5 values absence resolves to is not normative — css-color-adjust-1 §2 routes it through the UA default color scheme, which that spec leaves to the UA — solightis a choice, matching the web platform's. The rest is test and CI coverage: the platform media queries are revived, theglobalClassNamePolyfillgate gets a metro test,dynamicRootVariablesis added so a:roottest reaches the runtime, and the unit suite runs on Windows in CI.Tests
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.Mutation-proved: reverting the
src/diff and re-running these files alone turns 6 of 27 red. Six findings, six reds — each one is covered by a case that fails without it.Verification
tsc --noEmit0 errors ·eslint0 problems ·jest1074 passed, 3 failed, 19 skipped.The 3 are the Windows-only
babel-plugin-testerbaseline. The CI lane this adds is what would surface them upstream rather than only on a contributor's machine.Known limits
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.Base
Branched off
f70c402.mainhas since taken #451 (a5002c5). 5 of the 11 files this changes also moved there, and 2 genuinely conflict —src/__tests__/compiler/media-query.test.ts,src/web/api.tsx. Every measurement above was taken onf70c402. Say the word and I will re-apply it onto currentmain.