Skip to content

feat(core): warn when a variable value is not a declared option - #3121

Open
miguel-heygen wants to merge 1 commit into
mainfrom
feat/warn-unknown-enum-values
Open

feat(core): warn when a variable value is not a declared option#3121
miguel-heygen wants to merge 1 commit into
mainfrom
feat/warn-unknown-enum-values

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

What

Warn when a composition sets a variable to a value that is not one of that variable's declared options.

Why

The value silently falls back. The composition renders something the author did not ask for, and nothing anywhere says a choice was ignored.

Reported by a reviewer against the sub-composition case specifically, which was the silent one.

How

Both paths that resolve a variable warn, because they are genuinely separate:

  • Runtime covers a top-level composition.
  • Compile covers a sub-composition given instance values. Those are baked into the variables table at compile time, and the scoped getVariables shim only reads that table, so the runtime guard never runs there.

Both call the same helper, so the message and the per-process dedupe set are shared. An author sees one warning either way, worded identically whichever path produced it.

A warning, not an error, and the reason is symmetry: the same defect must not carry two severities depending on which mount path an author happened to use. The value does fall back to something renderable, and escalation already has a home in lint, which a project can make blocking in CI.

Deliberately not reusing validateVariables' enum branch — it also reports undeclared keys and type mismatches, which this path intentionally permits.

Scope is exactly the feature: getVariables +83, htmlBundler +9, inlineSubCompositions +9, plus tests.

Test plan

  • Unit tests added/updated
  • Manual testing performed
  • Documentation updated (if applicable)

packages/core: 99 files, 1716 tests passed, 0 failed. bun run lint exit 0. oxfmt --check clean on all three source files.

Tests were mutation-checked rather than trusted green. Breaking the guard in five distinct ways each killed at least one test:

Mutant Effect
Warning removed entirely broad
Dedupe removed duplicate warnings
Dedupe ignoring the value a second, different bad value goes unreported
Absent-value guard removed warns on values that were never set
Unknown value coerced to the default turns a diagnostic into a silent rewrite

The last one matters most: it would have made the fix quietly change what renders, which is worse than the bug.

Verified end to end on a real primitive mounted with a bad enum value, not only in unit tests. The message names the composition, the variable, the offending value, the allowed set, and what will render instead.

Worth knowing

check runs the prebuilt runtime from packages/core/dist, so probing this without rebuilding the runtime first reads stale behaviour and looks like the warning does not fire.

Not covered

The warning reaches the same console.warn channel as the bundler's other diagnostics, so it appears in terminal output but not in the structured check report or --json. #3104 threads a diagnostics sink into CheckReport and would carry this with it; the two touch htmlBundler.ts and should land in that order.

A composition can set a variable to a value outside that variable declared
enum options. The value silently falls back, the composition renders
something the author did not ask for, and no signal says a choice was
ignored.

Warns on both paths that resolve a variable, because they are separate. The
runtime guard covers a top-level composition. The compile guard covers a
sub-composition given instance values: those are baked into the variables
table at compile time and the scoped getVariables shim only reads that
table, so the runtime guard never runs there. That sub-comp case is the one
that motivated this, and it was the silent one.

Both call the same helper, so the message and the per-process dedupe set are
shared and an author sees one warning either way.

A warning rather than an error, for symmetry: the same defect must not carry
two severities depending on which mount path an author happened to use.
Escalation already has a home in lint, which a project can make blocking.

The check is split across four small helpers rather than one function:
parsing the declaration, naming the composition, reducing the option set to
comparable scalars, and deciding whether a value actually fell back. As one
function it audited at 21 cyclomatic and 25 cognitive.

Tests were mutation-checked. Five distinct breakages each killed a test,
including one that coerces the unknown value to the default instead of
warning, which would turn a diagnostic into a silent rewrite.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant