Skip to content

fix: make binding default values first class - #3342

Open
DylanPiercey wants to merge 1 commit into
mainfrom
first-class-default-values
Open

fix: make binding default values first class#3342
DylanPiercey wants to merge 1 commit into
mainfrom
first-class-default-values

Conversation

@DylanPiercey

@DylanPiercey DylanPiercey commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

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.

@changeset-bot

changeset-bot Bot commented Jul 10, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: fa8bcca

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@marko/runtime-tags Patch

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

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.00000% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.39%. Comparing base (b631eb5) to head (fa8bcca).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...ges/runtime-tags/src/translator/util/references.ts 86.11% 9 Missing and 1 partial ⚠️
packages/runtime-tags/src/translator/core/await.ts 44.44% 5 Missing ⚠️
...kages/runtime-tags/src/translator/util/get-root.ts 93.10% 2 Missing ⚠️
...e-tags/src/translator/util/strip-default-values.ts 98.83% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@DylanPiercey
DylanPiercey force-pushed the first-class-default-values branch from 850e689 to 3e21a90 Compare July 10, 2026 01:00
@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The 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)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main change: making binding default values first-class.
Description check ✅ Passed The description accurately describes the default-binding graph and DOM/HTML handling changes in the PR.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch first-class-default-values

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 0054d69 and 3e21a90.

⛔ Files ignored due to path filters (23)
  • packages/runtime-tags/src/__tests__/fixtures/content-params-array-defaults/__snapshots__/dom.bundle.debug.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/content-params-array-defaults/__snapshots__/dom.bundle.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/custom-tag-var-destructured/__snapshots__/html.bundle.debug.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/custom-tag-var-destructured/__snapshots__/html.bundle.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/dom.bundle.debug.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/dom.bundle.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/html.bundle.debug.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/html.bundle.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/render.debug.md is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/render.md is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/writes.debug.html is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/writes.html is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/param-destructure-default/__snapshots__/dom.bundle.debug.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/param-destructure-dynamic-default/__snapshots__/dom.bundle.debug.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/param-destructure-dynamic-default/__snapshots__/dom.bundle.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/dom.bundle.debug.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/dom.bundle.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/html.bundle.debug.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/html.bundle.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/render.debug.md is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/render.md is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/writes.debug.html is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/writes.html is excluded by !**/__snapshots__/** and included by **
📒 Files selected for processing (21)
  • .changeset/param-top-level-default.md
  • packages/runtime-tags/src/__tests__/fixtures/content-params-array-defaults/sizes.json
  • packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/sizes.json
  • packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/template.marko
  • packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/test.ts
  • packages/runtime-tags/src/__tests__/fixtures/param-destructure-dynamic-default/sizes.json
  • 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/template.marko
  • packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/test.ts
  • packages/runtime-tags/src/translator/core/await.ts
  • packages/runtime-tags/src/translator/core/for.ts
  • packages/runtime-tags/src/translator/util/get-root.ts
  • packages/runtime-tags/src/translator/util/known-tag.ts
  • packages/runtime-tags/src/translator/util/references.ts
  • packages/runtime-tags/src/translator/util/sections.ts
  • packages/runtime-tags/src/translator/util/strip-default-values.ts
  • packages/runtime-tags/src/translator/util/translate-attrs.ts
  • packages/runtime-tags/src/translator/util/translate-var.ts
  • packages/runtime-tags/src/translator/visitors/program/dom.ts
  • packages/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

Comment thread .changeset/param-top-level-default.md Outdated
"@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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.

Suggested change
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

Comment thread packages/runtime-tags/src/translator/util/references.ts
@DylanPiercey
DylanPiercey force-pushed the first-class-default-values branch 8 times, most recently from 1b5004e to a4eac2b Compare July 11, 2026 01:50
@DylanPiercey
DylanPiercey force-pushed the first-class-default-values branch from a4eac2b to 6058d04 Compare July 18, 2026 01:30
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.
@DylanPiercey
DylanPiercey force-pushed the first-class-default-values branch from 6058d04 to fa8bcca Compare July 18, 2026 01:34

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
packages/runtime-tags/src/translator/util/signals.ts (1)

930-948: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Place the exported API before its private helper.

Move buildResumeRegisterKey below getResumeRegisterId; 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

📥 Commits

Reviewing files that changed from the base of the PR and between 6058d04 and fa8bcca.

⛔ Files ignored due to path filters (34)
  • packages/runtime-tags/src/__tests__/fixtures/const-array-pattern/__snapshots__/dom.bundle.debug.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/content-params-array-defaults/__snapshots__/dom.bundle.debug.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/content-params-array-defaults/__snapshots__/dom.bundle.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/custom-tag-var-destructured/__snapshots__/dom.bundle.debug.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/custom-tag-var-destructured/__snapshots__/dom.bundle.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/custom-tag-var-destructured/__snapshots__/html.bundle.debug.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/custom-tag-var-destructured/__snapshots__/html.bundle.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/dom.bundle.debug.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/dom.bundle.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/html.bundle.debug.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/html.bundle.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/render.debug.md is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/render.md is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/writes.debug.html is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/__snapshots__/writes.html is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/destructure-unused-default/__snapshots__/dom.bundle.debug.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/destructure-unused-default/__snapshots__/dom.bundle.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/destructure-unused-default/__snapshots__/html.bundle.debug.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/destructure-unused-default/__snapshots__/html.bundle.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/destructure-unused-default/__snapshots__/render.debug.md is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/destructure-unused-default/__snapshots__/render.md is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/destructure-unused-default/__snapshots__/writes.debug.html is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/destructure-unused-default/__snapshots__/writes.html is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/function-tag-var-default-registration/__snapshots__/dom.bundle.debug.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/function-tag-var-default-registration/__snapshots__/dom.bundle.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/param-destructure-default/__snapshots__/dom.bundle.debug.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/dom.bundle.debug.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/dom.bundle.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/html.bundle.debug.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/html.bundle.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/render.debug.md is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/render.md is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/writes.debug.html is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/__snapshots__/writes.html is excluded by !**/__snapshots__/** and included by **
📒 Files selected for processing (25)
  • .changeset/param-top-level-default.md
  • packages/runtime-tags/src/__tests__/fixtures/content-params-array-defaults/sizes.json
  • packages/runtime-tags/src/__tests__/fixtures/custom-tag-var-destructured/sizes.json
  • packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/sizes.json
  • packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/template.marko
  • packages/runtime-tags/src/__tests__/fixtures/destructure-alias-default/test.ts
  • packages/runtime-tags/src/__tests__/fixtures/destructure-unused-default/sizes.json
  • packages/runtime-tags/src/__tests__/fixtures/destructure-unused-default/template.marko
  • packages/runtime-tags/src/__tests__/fixtures/destructure-unused-default/test.ts
  • 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/template.marko
  • packages/runtime-tags/src/__tests__/fixtures/param-top-level-default/test.ts
  • packages/runtime-tags/src/translator/core/await.ts
  • packages/runtime-tags/src/translator/core/for.ts
  • packages/runtime-tags/src/translator/index.ts
  • packages/runtime-tags/src/translator/util/get-root.ts
  • packages/runtime-tags/src/translator/util/known-tag.ts
  • packages/runtime-tags/src/translator/util/references.ts
  • packages/runtime-tags/src/translator/util/signals.ts
  • packages/runtime-tags/src/translator/util/strip-default-values.ts
  • packages/runtime-tags/src/translator/util/translate-attrs.ts
  • packages/runtime-tags/src/translator/util/translate-var.ts
  • packages/runtime-tags/src/translator/visitors/assignment-pattern.ts
  • packages/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

Comment on lines +1110 to +1114
const target = canDefaultValueParam(derivedSignal)
? derivedSignal
: sourceSignal;
target.valueParamDefault = entry.value.alternate;
entry.value = entry.value.consequent;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
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.

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