fix: make binding default values first class - #3342
Conversation
🦋 Changeset detectedLatest commit: fa8bcca The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3342 +/- ##
========================================
Coverage 94.39% 94.39%
========================================
Files 389 391 +2
Lines 54157 54419 +262
Branches 4449 4496 +47
========================================
+ Hits 51120 51371 +251
- Misses 3004 3018 +14
+ Partials 33 30 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
850e689 to
3e21a90
Compare
WalkthroughThe change adds compiler tracking for defaulted bindings, including default sources, derived values, and resumable section metadata. HTML translation extracts default expressions into generated statements for tag variables, content, await bodies, and for bodies. DOM parameter signal generation handles omitted parameters explicitly. Pre-analysis destructuring insertions are removed. New regression fixtures cover top-level parameters, destructured aliases, unused defaults, dynamic updates, and generated size metadata. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/param-top-level-default.md:
- Line 5: Update the changeset description to hyphenate the compound modifiers,
replacing “top level” with “top-level” and “client side” with “client-side.”
In `@packages/runtime-tags/src/translator/util/references.ts`:
- Around line 773-853: Clone the AssignmentPattern default expression before
reusing it in the generated declaration. In the AssignmentPattern handling
block, especially where addSetupExpr and defaultedValues use right, ensure
stripDefaultValues receives or inserts a cloned lVal.right node so the original
pattern and new declaration never share the same AST node object.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b64259dc-5cc7-4636-afaa-9204a7e0707a
⛔ Files ignored due to path filters (23)
packages/runtime-tags/src/__tests__/fixtures/content-params-array-defaults/__snapshots__/dom.bundle.debug.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/content-params-array-defaults/__snapshots__/dom.bundle.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/custom-tag-var-destructured/__snapshots__/html.bundle.debug.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/custom-tag-var-destructured/__snapshots__/html.bundle.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/dom.bundle.debug.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/dom.bundle.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/html.bundle.debug.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/html.bundle.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/render.debug.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/render.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/writes.debug.htmlis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/writes.htmlis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/param-destructure-default/__snapshots__/dom.bundle.debug.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/param-destructure-dynamic-default/__snapshots__/dom.bundle.debug.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/param-destructure-dynamic-default/__snapshots__/dom.bundle.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/dom.bundle.debug.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/dom.bundle.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/html.bundle.debug.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/html.bundle.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/render.debug.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/render.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/writes.debug.htmlis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/writes.htmlis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (21)
.changeset/param-top-level-default.mdpackages/runtime-tags/src/__tests__/fixtures/content-params-array-defaults/sizes.jsonpackages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/sizes.jsonpackages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/template.markopackages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/test.tspackages/runtime-tags/src/__tests__/fixtures/param-destructure-dynamic-default/sizes.jsonpackages/runtime-tags/src/__tests__/fixtures/param-top-level-default/sizes.jsonpackages/runtime-tags/src/__tests__/fixtures/param-top-level-default/tags/child.markopackages/runtime-tags/src/__tests__/fixtures/param-top-level-default/template.markopackages/runtime-tags/src/__tests__/fixtures/param-top-level-default/test.tspackages/runtime-tags/src/translator/core/await.tspackages/runtime-tags/src/translator/core/for.tspackages/runtime-tags/src/translator/util/get-root.tspackages/runtime-tags/src/translator/util/known-tag.tspackages/runtime-tags/src/translator/util/references.tspackages/runtime-tags/src/translator/util/sections.tspackages/runtime-tags/src/translator/util/strip-default-values.tspackages/runtime-tags/src/translator/util/translate-attrs.tspackages/runtime-tags/src/translator/util/translate-var.tspackages/runtime-tags/src/translator/visitors/program/dom.tspackages/runtime-tags/src/translator/visitors/program/pre-analyze.ts
💤 Files with no reviewable changes (1)
- packages/runtime-tags/src/translator/visitors/program/pre-analyze.ts
| "@marko/runtime-tags": patch | ||
| --- | ||
|
|
||
| Fix default values on top level tag parameters (eg `<my-tag|a, b = a|>`) being dropped in the DOM output, which caused a crash or stale content client side. Default values in tag parameters and destructured tag variables are now represented uniformly in the compiler and defaulted values are consistently resumable. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hyphenate the compound modifiers.
Use “top-level” and “client-side” in the changeset description.
Suggested wording
-Fix default values on top level tag parameters ...
+Fix default values on top-level tag parameters ...
-... stale content client side.
+... stale content client-side.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Fix default values on top level tag parameters (eg `<my-tag|a, b = a|>`) being dropped in the DOM output, which caused a crash or stale content client side. Default values in tag parameters and destructured tag variables are now represented uniformly in the compiler and defaulted values are consistently resumable. | |
| Fix default values on top-level tag parameters (eg `<my-tag|a, b = a|>`) being dropped in the DOM output, which caused a crash or stale content client-side. Default values in tag parameters and destructured tag variables are now represented uniformly in the compiler and defaulted values are consistently resumable. |
🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: Use a hyphen to join words.
Context: ...s": patch --- Fix default values on top level tag parameters (eg `<my-tag|a, b =...
(QB_NEW_EN_HYPHEN)
[grammar] ~5-~5: Use a hyphen to join words.
Context: ...h caused a crash or stale content client side. Default values in tag parameters a...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.changeset/param-top-level-default.md at line 5, Update the changeset
description to hyphenate the compound modifiers, replacing “top level” with
“top-level” and “client side” with “client-side.”
Source: Linters/SAST tools
1b5004e to
a4eac2b
Compare
a4eac2b to
6058d04
Compare
Default values in tag params and destructured tag variables are modeled directly in the binding graph: a source binding holds the value applied at the pattern's position, and the declared binding is derived from it. The DOM output registers the derivation as a signal value, collapsing it to a native parameter default when the fallback needs no state; the HTML output materializes the source variable so it stays serializable for resume. The alias root of a shared property always holds the raw value, so aliased destructures agree regardless of which side has a default.
6058d04 to
fa8bcca
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/runtime-tags/src/translator/util/signals.ts (1)
930-948: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlace the exported API before its private helper.
Move
buildResumeRegisterKeybelowgetResumeRegisterId; function-declaration hoisting preserves behavior while maintaining progressive disclosure.As per coding guidelines, organize TypeScript files with “public APIs and exports first” and use function-declaration hoisting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/runtime-tags/src/translator/util/signals.ts` around lines 930 - 948, Move the buildResumeRegisterKey function declaration below getResumeRegisterId in signals.ts, leaving its implementation and behavior unchanged. Keep the exported public API before this private helper, relying on function-declaration hoisting.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/runtime-tags/src/translator/util/signals.ts`:
- Around line 1110-1114: Update the assignment in the derived-signal translation
logic so valueParamDefault is applied only when
canDefaultValueParam(derivedSignal) is true; when ineligible, do not assign the
fallback to sourceSignal, preserving raw undefined forwarding to aliases and
subscribers.
---
Nitpick comments:
In `@packages/runtime-tags/src/translator/util/signals.ts`:
- Around line 930-948: Move the buildResumeRegisterKey function declaration
below getResumeRegisterId in signals.ts, leaving its implementation and behavior
unchanged. Keep the exported public API before this private helper, relying on
function-declaration hoisting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac8d4850-b8c5-4cf1-a09b-205d8c06519d
⛔ Files ignored due to path filters (34)
packages/runtime-tags/src/__tests__/fixtures/const-array-pattern/__snapshots__/dom.bundle.debug.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/content-params-array-defaults/__snapshots__/dom.bundle.debug.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/content-params-array-defaults/__snapshots__/dom.bundle.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/custom-tag-var-destructured/__snapshots__/dom.bundle.debug.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/custom-tag-var-destructured/__snapshots__/dom.bundle.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/custom-tag-var-destructured/__snapshots__/html.bundle.debug.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/custom-tag-var-destructured/__snapshots__/html.bundle.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/dom.bundle.debug.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/dom.bundle.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/html.bundle.debug.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/html.bundle.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/render.debug.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/render.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/writes.debug.htmlis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/writes.htmlis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/destructure-unused-default/__snapshots__/dom.bundle.debug.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/destructure-unused-default/__snapshots__/dom.bundle.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/destructure-unused-default/__snapshots__/html.bundle.debug.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/destructure-unused-default/__snapshots__/html.bundle.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/destructure-unused-default/__snapshots__/render.debug.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/destructure-unused-default/__snapshots__/render.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/destructure-unused-default/__snapshots__/writes.debug.htmlis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/destructure-unused-default/__snapshots__/writes.htmlis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/function-tag-var-default-registration/__snapshots__/dom.bundle.debug.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/function-tag-var-default-registration/__snapshots__/dom.bundle.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/param-destructure-default/__snapshots__/dom.bundle.debug.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/dom.bundle.debug.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/dom.bundle.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/html.bundle.debug.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/html.bundle.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/render.debug.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/render.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/writes.debug.htmlis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/writes.htmlis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (25)
.changeset/param-top-level-default.mdpackages/runtime-tags/src/__tests__/fixtures/content-params-array-defaults/sizes.jsonpackages/runtime-tags/src/__tests__/fixtures/custom-tag-var-destructured/sizes.jsonpackages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/sizes.jsonpackages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/template.markopackages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/test.tspackages/runtime-tags/src/__tests__/fixtures/destructure-unused-default/sizes.jsonpackages/runtime-tags/src/__tests__/fixtures/destructure-unused-default/template.markopackages/runtime-tags/src/__tests__/fixtures/destructure-unused-default/test.tspackages/runtime-tags/src/__tests__/fixtures/param-top-level-default/sizes.jsonpackages/runtime-tags/src/__tests__/fixtures/param-top-level-default/tags/child.markopackages/runtime-tags/src/__tests__/fixtures/param-top-level-default/template.markopackages/runtime-tags/src/__tests__/fixtures/param-top-level-default/test.tspackages/runtime-tags/src/translator/core/await.tspackages/runtime-tags/src/translator/core/for.tspackages/runtime-tags/src/translator/index.tspackages/runtime-tags/src/translator/util/get-root.tspackages/runtime-tags/src/translator/util/known-tag.tspackages/runtime-tags/src/translator/util/references.tspackages/runtime-tags/src/translator/util/signals.tspackages/runtime-tags/src/translator/util/strip-default-values.tspackages/runtime-tags/src/translator/util/translate-attrs.tspackages/runtime-tags/src/translator/util/translate-var.tspackages/runtime-tags/src/translator/visitors/assignment-pattern.tspackages/runtime-tags/src/translator/visitors/program/pre-analyze.ts
💤 Files with no reviewable changes (1)
- packages/runtime-tags/src/translator/visitors/program/pre-analyze.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/runtime-tags/src/tests/fixtures/param-top-level-default/sizes.json
- packages/runtime-tags/src/tests/fixtures/param-top-level-default/tags/child.marko
- packages/runtime-tags/src/tests/fixtures/param-top-level-default/test.ts
- packages/runtime-tags/src/tests/fixtures/destructure-alias-default/test.ts
- packages/runtime-tags/src/tests/fixtures/content-params-array-defaults/sizes.json
- packages/runtime-tags/src/translator/core/await.ts
- packages/runtime-tags/src/translator/util/translate-attrs.ts
- packages/runtime-tags/src/translator/util/translate-var.ts
- packages/runtime-tags/src/translator/core/for.ts
- packages/runtime-tags/src/tests/fixtures/destructure-alias-default/template.marko
- packages/runtime-tags/src/translator/util/strip-default-values.ts
- packages/runtime-tags/src/translator/util/get-root.ts
- packages/runtime-tags/src/tests/fixtures/custom-tag-var-destructured/sizes.json
- packages/runtime-tags/src/translator/util/known-tag.ts
| const target = canDefaultValueParam(derivedSignal) | ||
| ? derivedSignal | ||
| : sourceSignal; | ||
| target.valueParamDefault = entry.value.alternate; | ||
| entry.value = entry.value.consequent; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the fallback on the derived signal.
When derivedSignal is ineligible, assigning sourceSignal.valueParamDefault converts raw undefined before forwarding to every alias and subscriber. This breaks the raw-value alias contract. Leave the conditional intact unless the derived signal can own the parameter default.
Proposed fix
- if (entry && entry.value.type === "ConditionalExpression") {
- const target = canDefaultValueParam(derivedSignal)
- ? derivedSignal
- : sourceSignal;
- target.valueParamDefault = entry.value.alternate;
+ if (
+ entry &&
+ entry.value.type === "ConditionalExpression" &&
+ canDefaultValueParam(derivedSignal)
+ ) {
+ derivedSignal.valueParamDefault = entry.value.alternate;
entry.value = entry.value.consequent;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const target = canDefaultValueParam(derivedSignal) | |
| ? derivedSignal | |
| : sourceSignal; | |
| target.valueParamDefault = entry.value.alternate; | |
| entry.value = entry.value.consequent; | |
| if ( | |
| entry && | |
| entry.value.type === "ConditionalExpression" && | |
| canDefaultValueParam(derivedSignal) | |
| ) { | |
| derivedSignal.valueParamDefault = entry.value.alternate; | |
| entry.value = entry.value.consequent; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/runtime-tags/src/translator/util/signals.ts` around lines 1110 -
1114, Update the assignment in the derived-signal translation logic so
valueParamDefault is applied only when canDefaultValueParam(derivedSignal) is
true; when ineligible, do not assign the fallback to sourceSignal, preserving
raw undefined forwarding to aliases and subscribers.
Default values in tag params and destructured tag variables are modeled directly in the binding graph: a source binding holds the value applied at the pattern's position, and the declared binding is derived from it. The DOM output registers the derivation as a signal value, collapsing it to a native parameter default (
($scope, $x = fallback) =>) when the fallback needs no state; the HTML output materializes the source variable so it stays serializable for resume. The alias root of a shared property always holds the raw value, so aliased destructures agree regardless of which side has a default. A defaulted binding that is never read prunes together with its fallback and source, so unused defaults leave no trace in the DOM output.