feat(metro): surface compiler warnings during a build - #430
Open
YevheniiKotyrlo wants to merge 8 commits into
Open
feat(metro): surface compiler warnings during a build#430YevheniiKotyrlo wants to merge 8 commits into
YevheniiKotyrlo wants to merge 8 commits into
Conversation
The import plugin's relative-import handlers resolve a source against the file being transformed and then match the result against forward-slash literals (`react-native/Libraries/Components/`, `react-native-web/dist`). path.resolve returns backslash-separated paths on Windows, so those splits never match and the import is left un-rewritten. Only relative imports break, and only on Windows -- bare specifiers are plain string matches; the failing cases were green on the Linux/macOS CI. Add a `resolvePosix` helper (resolve + normalize \ -> /) and use it in parseReactNativeSource / parseReactNativeWebSource. Resolve semantics are unchanged; only the separator is normalized. import-plugin's isFromThisModule stays on path.resolve -- it compares two OS-native paths, so it already works. Adds a helpers unit test asserting the POSIX invariant.
`resolvePosix` fused `path.resolve` into the helper, so every test of it had to go through `resolve` — and on Linux `resolve` never emits a backslash. The normalization had nothing to act on there, so the tests passed with it deleted. CI runs ubuntu-latest and nothing else, which is how the three failing cases sat in main unnoticed. `toPosixPath` is the primitive now and `resolvePosix` composes with it, so a test can feed it a Windows-shaped literal and fail on any host. Deleting the normalization turns 8 tests red across 3 suites, three of them host-independent. Left unconditional rather than gated on `sep`. Gating is tempting — a POSIX filename may legally contain a backslash — but it makes the function an identity on Linux and puts the guard back out of CI's reach. The hazard it would close needs a directory literally named `react-native\Libraries\Components\` on a POSIX host, and the failure mode is a missed rewrite rather than wrong output.
A backslash is a legal filename character on POSIX, so rewriting one there corrupts a path that was already correct. The gate belongs at the boundary where a host path enters — resolvePosix — rather than inside toPosixPath, which stays a pure transform so a test can feed it a Windows-shaped literal and observe the result on any host.
…ards
The Windows separator fix stands: `resolveImportSource` normalizes
`path.resolve`'s output to POSIX before the handlers match it against
forward-slash literals. The host separator is now an argument to
`toPosixPath` rather than a module-level `sep` read, so both branches are
reachable from any host — CI runs only ubuntu-latest and one macos-15, and
an assertion driven through `path.resolve` is vacuous there.
Auditing that path turned up three more defects in the same few lines.
`react-native-web.ts` resolved a relative source against the filename where
it meant the filename's directory, consuming one `..` too few and moving the
package boundary by a directory; `react-native.ts` already used `dirname`.
Both now call one helper whose signature gives the caller no base to get
wrong.
`processed.has(path)` could never be true: only `Statement` nodes are added
to that set and a `NodePath` is not one. Throw-injecting it leaves the whole
suite green, while the same probe on `path.node` trips on nearly every
rewrite — that sibling is the live re-entry guard. The dead disjunct is gone
and the set is typed `WeakSet<Statement>`, so re-adding it is a compile
error.
`isFromThisModule` derived the package root as `../../../` from `__dirname`,
which names it in the built layout and points outside the package when the
plugin runs from `src/`. It also read `.startsWith` off `state.filename`,
which babel types `string | undefined` and leaves undefined when a caller
passes none — that threw before any rewrite was considered. The root now
comes from the nearest `package.json` declaring a `name` (builder-bob writes
a bare `{ "type": ... }` manifest into each output directory), each shipped
directory is compared with a trailing separator, and the filename is
narrowed once per visitor.
That guard being live is why three existing suites change: babel-plugin-tester
infers `filepath` from the test file's own path, and a file under
`src/__tests__/` genuinely is one of this package's sources. Their
`babelOptions.filename` never reached babel at all. Each now sets `filepath`
to an application path, which is what those cases always meant.
Tests: the plugin end-to-end through `transformSync`, this package's own
sources in both layouts, the package-boundary cases, and the first coverage
of the metro resolver plane — pinned against the babel plane over the census
they share, since the two are alternatives selected by
`globalClassNamePolyfill` and must agree.
`plugin.test.mts` is deleted. Jest 29 collects neither the `.mts` extension
nor that testMatch shape, its first case carries `only: true`, and that case
expects output the plugin does not emit. The two shapes no collected suite
covered are ported over with the expectations the plugin actually produces.
`getWarnings` declared its return type inline, so every consumer of the public `compile(css).warnings()` result had to restate the three channels or reach for `ReturnType<...>`. `CompilerWarnings` is that shape, exported from the compiler's public types beside `CompilerOptions`. No behaviour changes: the object built and returned is identical.
The compiler records every declaration it cannot translate — 52
`addWarning` call sites in `declarations.ts` — and `compile(css).warnings()`
hands them back. Nothing in a real build ever asked: `metro-transformer.ts`
called `.stylesheet()` and dropped the compile result, so the only consumer
was `src/jest/index.ts`, behind `REACT_NATIVE_CSS_TEST_DEBUG` + `--inspect`.
An unsupported declaration therefore did nothing, silently, and the compiler
knew and said so into a void.
The transformer now reads them and writes one block per stylesheet:
react-native-css: src/global.css - 3 declarations dropped, no React Native equivalent
properties: columns, float
values: z-index: auto
The noise model is the part worth reviewing.
Metro re-transforms a file whenever its contents change, and a Tailwind
build rewrites its CSS output on nearly every source save, so printing
unconditionally would reprint the same block on every keystroke. A block is
keyed by the file and the message, so it prints when a file's set of
warnings CHANGES — the first compile, and thereafter only when a fix removes
an entry or a new declaration adds one. The map is bounded by the number of
CSS files rather than by the number of transforms.
Volume is capped for the same reason. Compiling the Tailwind corpus in
`src/__tests__/vendor/tailwind` warns on 87 distinct properties across 389
of its cases, so a `summary` lists ten entries per channel, five values per
property, 60 characters per value, and counts the rest. `warnings: "verbose"`
lifts every cap; `warnings: "none"` prints nothing.
`console.warn` is what makes it reachable: Metro pipes each transform
worker's stderr to the parent and its reporter prints the chunk, and with
`maxWorkers: 1` the worker is required in-band. Neither path fails a build —
these are advisory, and a dropped `float` is not a reason to refuse to
bundle. The block carries no ANSI escapes because a piped worker has no TTY,
so the vendored picocolors would disable itself in exactly the environment
this runs in.
Native only. Web returns to the stock Expo transformer before the compiler
is reached, so a web bundle produces no warnings to surface; the test asserts
that the CSS was still processed whole rather than that nothing happened.
Tested on both planes. `warnings.test.ts` drives the formatter and the
reporter, taking its fixtures from real `compile()` calls so a compiler that
stopped warning would fail them. `metro-transformer.test.ts` runs the real
transformer against the real Expo transform worker and asserts the block
lands on `console.warn` — the assertion that has never been true.
Closes nativewind#424
…duces
This pull request surfaces what the compiler knows it could not translate. It
was silent about a fourth thing it also knows: input lightningcss could not
parse.
lightningcss has two classes of malformed CSS. One throws, and a throw is
already loud. The other is recovered and reported through `result.warnings`
with no flag needed — and both `lightningcss()` calls here discarded that
return, the first by destructuring only `code` and the second by ignoring it
entirely.
The consequence is worse than a missing message, because of where the rule is
lost. lightningcss passes the malformed sheet through verbatim; it is this
package's own visitor that then finds nothing to extract. So the rule silently
disappears and lightningcss's warning is the only evidence it ever existed:
@unknown-thing { .b { color: blue } } .b vanishes, warnings() {}
.a::wat { color: red } .a vanishes, warnings() {}
Both are ordinary typos, and an unsupported PROPERTY in the same position does
report — so the gap was specifically the syntax class.
Only the FIRST pass is read. The second re-parses the first's output and,
measured against both triggers, returns the identical message; reading it would
add nothing but a duplicate to suppress.
Warnings are collected in a `Set`. lightningcss emits one per occurrence and
the message carries no line or column, so two copies of one mistake are a
string the reader cannot tell apart from itself. Two DISTINCT mistakes still
report twice, and the test asserts both halves.
`OWN_AT_RULE_WARNINGS` filters the two at-rules this package defines —
`@react-native` and `@nativeMapping`, the complete set from `atRules.ts`.
lightningcss calls both unknown because they are ours, so reporting them would
fire on every stylesheet this compiler is built to read.
Tailwind is not a noise source here, measured rather than assumed. Every v4
authoring at-rule warns at the lightningcss layer, but none reach this
compiler: `metro-transformer.ts` runs the Expo/Metro CSS worker first and
compiles its output, by which point PostCSS has consumed them. Over 24 utility
classes plus an inline `@theme` / `@utility` / `@custom-variant` / `@source`
block, in both optimisation modes: zero surviving at-rules, zero warnings.
Deliberately NOT paired with lightningcss's `errorRecovery`. That converts the
throwing class into more of this one, which changes what compiles rather than
what is reported — and it drops more than it reports: one probe kept a single
warning while silently discarding two further rules. It belongs in its own
change, argued on its own terms.
The previous commit collected syntax warnings end to end and rendered them
nowhere. `formatCompilerWarnings` summed only `properties`, `functions` and
`values`, so a stylesheet whose ONLY problem was malformed CSS returned
`undefined` and the build printed nothing — the exact silence the channel was
added to remove.
It shipped green because every test for the channel read the PRODUCER
(`compiled.warnings().syntax`) and none reached the formatter. A dead renderer
under a live producer is invisible to that shape of test, which is why both new
cases go through `formatCompilerWarnings`.
Syntax is counted and reported SEPARATELY rather than folded into the dropped
count, because the two are different claims. A dropped declaration is CSS this
package cannot EXPRESS; a syntax warning is CSS lightningcss could not PARSE.
They send a reader to different places — one to this package's limits, one to
their own stylesheet — so a syntax warning under a header reading "no React
Native equivalent" misdirects the fix. The header now carries whichever claims
apply:
- 1 rule could not be parsed
syntax: Unknown at rule: @unknown-thing
- 1 declaration dropped, no React Native equivalent; 1 rule could not be parsed
syntax: Unknown at rule: @unknown-thing
properties: float
The overflow total counts the channel too, so `(+N more)` no longer
under-reports when syntax entries are capped.
Suite 1197 passed / 0 failed. Dropping the syntax line turns both new tests red.
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
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.
Closes #424.
What was wrong
compile(css).warnings()collects everything the compiler could not translate — 52addWarningcall sites insrc/compiler/declarations.ts.src/metro/metro-transformer.tscalled.stylesheet()and threw the compile result away, so the only consumer in the tree wassrc/jest/index.ts, behindREACT_NATIVE_CSS_TEST_DEBUG+--inspect.The observable behaviour of an unsupported declaration was therefore: nothing happens, silently. The compiler knew, wrote it down, and dropped the note at the transform boundary.
What this does
The transformer reads
.warnings()and writes one block per stylesheet:Issue #424 offered three shapes and you had not picked one, so this is option 1 with option 2's escape hatch: on by default, one capped block per file,
warnings: "none"to silence. The reasoning is that an opt-in flag leaves the channel unreachable for everyone who does not read the docs, which is the thing the issue is about. If you would rather it defaulted to"none", that is a one-word change inreportCompilerWarningsand I will make it.The noise model, and why
Metro re-transforms a file whenever its contents change, and a Tailwind build rewrites its CSS output on nearly every source save — so printing unconditionally would reprint an identical block on every keystroke for a whole session.
A block is keyed by the file and by the message it would print, so it appears when a file's set of warnings changes: the first compile, and thereafter only when a fix removes an entry or a new declaration adds one. Fix
floatand the next block is printed without it. The map holds one entry per CSS file, not one per transform, and lives per worker process, so restarting the bundler reprints.Volume is capped, on a measured worst case. Compiling the Tailwind corpus already in this repo (
src/__tests__/vendor/tailwind) has 387 of its 759 cases asserting a warning, over 87 distinct properties — an uncapped block would be a screenful on the single most common setup this package has. Asummarytherefore lists ten entries per channel, five values per property and 60 characters per value, then counts the rest and says how to see it.verboselifts every cap.Nothing fails a build. These are advisory; a dropped
floatis not a reason to refuse to bundle.Delivery, and why
console.warnA transformer cannot reach Metro's reporter —
JsTransformerConfigdoes not carry one, and the transform runs in ajest-workerchild. It does not need to:WorkerFarmpipes each worker's stderr to the parent and forwards it asworker_stderr_chunk, whichTerminalReporter._logWorkerChunkprints. WithmaxWorkers: 1the worker is required in-band and the write lands on the bundler's own stderr directly. Both paths reach the terminal.That is also why the block carries no ANSI escapes: a piped worker has no TTY, so the vendored
picocolorsdisables itself in exactly the environment this code runs in, and colour would appear only in the in-bandmaxWorkers: 1case. Consistently plain beats inconsistently coloured.Platforms
Native only, and web carries no path at all rather than being skipped.
metro-transformer.tsreturns to the stock Expo transformer forplatform === "web"before the compiler is reached, so a web bundle produces no warnings to surface. The test for it asserts both halves — that nothing was reported, and that the stock transformer still emitted the CSS whole (floatincluded) — so the silence is not silence from having done nothing.Tests
src/__tests__/metro/gains two files, 32 tests.warnings.test.ts(24) drives the formatter and the reporter. Every fixture is a realcompile()result rather than a hand-writtenCompilerWarningsliteral, so a compiler that stopped warning fails these too. The one literal is thefunctionschannel, whichgetWarnings()declares but noaddWarningcall site writes to today — the test says so, and exists so the first producer is not born unreachable.metro-transformer.test.ts(8) runs the realtransform()against the real Expo transform worker, real lightningcss and the real compiler, and asserts the block lands onconsole.warn. That is the assertion that has never been true: deleting thereportCompilerWarningscall — i.e. restoringmain— turns four of these red.Every one of the 27 was mutation-proved: 22 mutations of the source, each run to confirm which tests it kills, every test killed by at least one. The set includes "the transformer never reports", "no per-file dedupe", "the dedupe is once per process", "verbose does not lift the cap", "web takes the native path" and "the path is always absolute".
Second commit — the fourth channel: diagnostics lightningcss already produced
The three channels above are things this compiler understood and cannot express. There is a fourth thing it knows and was throwing away: input lightningcss could not parse.
lightningcss has two classes of malformed CSS. One throws, and a throw is already loud. The other is recovered and reported through
result.warningswith no flag needed — and bothlightningcss()calls discarded that return, the first by destructuring onlycode, the second by ignoring it entirely.What makes this worth a channel rather than a shrug is where the rule is lost. lightningcss passes the malformed sheet through verbatim; it is this package's own visitor that then finds nothing to extract. So the rule silently disappears and lightningcss's warning is the only evidence it existed:
Both are ordinary typos. An unsupported property in the same position already reported (
{"properties":["float"]}), so the gap was specifically the syntax class.Four decisions, each measured rather than assumed:
Set, not an array. lightningcss emits one warning per occurrence and the message carries no line or column, so two copies of one mistake are a string the reader cannot tell apart from itself. Two distinct mistakes still report twice, and the test asserts both halves — a Set→array mutation turns it red.OWN_AT_RULE_WARNINGSfilters@react-nativeand@nativeMapping, the complete set fromatRules.ts. lightningcss calls both unknown because they are ours; reporting them would fire on every stylesheet this compiler is built to read.errorRecovery. That flag converts the throwing class into more of this one — a change to what compiles, not to what is reported — and it drops more than it reports: one probe kept a single warning while silently discarding two further rules. It belongs in its own change.Tailwind is not a noise source, measured. Every v4 authoring at-rule (
@tailwind,@theme,@apply,@plugin,@source,@custom-variant,@utility) warns at the lightningcss layer, but none reach this compiler:metro-transformer.tsruns the Expo/Metro CSS worker first and compiles its output, by which point PostCSS has consumed them. Over 24 utility classes plus an inline@theme/@utility/@custom-variant/@sourceblock, in both optimisation modes — zero surviving at-rules, zero warnings.Checks
yarn testtwice from a cold tree, identical both times:numFailedTests: 0,numRuntimeErrorTestSuites: 0. The base (#390's branch) measured1163 passed, 1184 totalacross 63 suites, so this adds 32 tests and 2 suites and breaks nothing.yarn typecheckandyarn lintboth exit 0.Also in here
compile(css).warnings()returned an inline anonymous type, so a consumer had to restate the channels or reach forReturnType<...>. That shape is nowCompilerWarnings, exported from the compiler's public types besideCompilerOptions. Separate commit, no behaviour change.