Skip to content

Conversation

@DylanPiercey
Copy link
Contributor

Description

Fixes a regression caused by #3021 which caused the compiler to error when attempting to write out the reads for some signals.

Also refactor and cleanup the signal construction and hasSideEffect logic a bit.

@changeset-bot
Copy link

changeset-bot bot commented Jan 8, 2026

🦋 Changeset detected

Latest commit: 7acdc2c

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

@coderabbitai
Copy link

coderabbitai bot commented Jan 8, 2026

Walkthrough

The changes fix a bug in the Marko compiler where section information was not stored during scope reads. A new changeset documents the patch for @marko/runtime-tags. The fix modifies how references track section metadata by explicitly storing section information in the reference extra object. The createScopeReadExpression function signature is updated to default the section parameter to reference.section, and the Signal interface is refactored to use concrete boolean fields and union-undefined types with adjusted side-effect propagation logic. A new test fixture template is added.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: storing section information with scope reads, which directly addresses the regression fix described in the PR.
Description check ✅ Passed The description is directly related to the changeset, explaining the regression fix from PR #3021 and the signal construction refactoring that the changes implement.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

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

63-86: Excellent refactoring to explicit interface fields.

Converting Signal from a type to an interface and using concrete boolean fields (hasDynamicSubscribers, hasSideEffect) instead of optional fields improves type safety and code clarity. The explicit undefined unions for extraArgs, prependStatements, and buildAssignment make the API contract clearer.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 59a90bf and 7acdc2c.

⛔ Files ignored due to path filters (10)
  • packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-and-update/__snapshots__/.name-cache.json is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-and-update/__snapshots__/csr-sanitized.expected.md is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-and-update/__snapshots__/csr.expected.md is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-and-update/__snapshots__/dom.expected/template.hydrate.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-and-update/__snapshots__/dom.expected/template.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-and-update/__snapshots__/html.expected/template.js is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-and-update/__snapshots__/resume-sanitized.expected.md is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-and-update/__snapshots__/resume.expected.md is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-and-update/__snapshots__/ssr-sanitized.expected.md is excluded by !**/__snapshots__/** and included by **
  • packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-and-update/__snapshots__/ssr.expected.md is excluded by !**/__snapshots__/** and included by **
📒 Files selected for processing (5)
  • .changeset/eager-rabbits-live.md
  • packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-and-update/template.marko
  • packages/runtime-tags/src/translator/util/references.ts
  • packages/runtime-tags/src/translator/util/scope-read.ts
  • packages/runtime-tags/src/translator/util/signals.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: test: node@24
  • GitHub Check: test: node@22
  • GitHub Check: test: node@20
🔇 Additional comments (8)
.changeset/eager-rabbits-live.md (1)

1-5: LGTM!

The changeset properly documents the patch for @marko/runtime-tags and clearly describes the fix for storing section information during scope reads.

packages/runtime-tags/src/translator/util/references.ts (1)

263-265: LGTM! Core fix for the regression.

The introduction of the intermediate refExtra variable and explicit assignment of refExtra.section = refSection ensures that section information is properly stored during scope reads. This addresses the regression mentioned in the PR description where the compiler was erroring when attempting to write out reads for some signals.

The pattern of explicitly storing section metadata aligns with the related changes in scope-read.ts.

packages/runtime-tags/src/translator/util/scope-read.ts (3)

77-79: Good API improvement with default parameter.

The default parameter section = reference.section simplifies the calling convention and makes the typical case (where section matches reference.section) more explicit. This reduces the need for callers to pass undefined.


90-94: LGTM! Explicit section metadata attachment.

The conditional attachment of exprExtra.read and exprExtra.section ensures that section information is properly stored when the reference is in the same section and is not a DOM binding. This complements the fix in references.ts and ensures consistent section tracking across scope reads.


83-85: Clean scope selection logic.

The simplified ternary clearly distinguishes between local bindings (using scopeIdentifier) and non-local bindings (using getScopeExpression), improving code readability.

packages/runtime-tags/src/translator/util/signals.ts (2)

228-241: Clear and comprehensive signal initialization.

The explicit initialization of all Signal fields, particularly the inline computation of hasSideEffect based on binding characteristics, makes the behavior deterministic and easier to understand. The logic properly covers arrays, DOM bindings, let bindings, cross-section references, closures, and hoists.


604-622: Good centralization of side effect propagation.

Explicitly setting providerSignal.hasSideEffect = true when establishing subscriptions centralizes the side-effect determination logic. This is clearer than the previous approach where side effects were inferred across multiple code paths.

packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-and-update/template.marko (1)

1-5: Appropriate test fixture for the regression fix.

This test fixture properly exercises the scenario where the compiler was failing to store section information during scope reads. It combines:

  • A let binding (count)
  • A native tag reference (/btn)
  • Data attribute binding (data-count=count)
  • A script that updates the DOM (btn.textContent = "after")

This effectively validates that the fix correctly handles section information for native tag references and subsequent updates.

@codecov
Copy link

codecov bot commented Jan 8, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.13%. Comparing base (59a90bf) to head (7acdc2c).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3032      +/-   ##
==========================================
- Coverage   89.13%   89.13%   -0.01%     
==========================================
  Files         375      375              
  Lines       47020    47018       -2     
  Branches     4023     4019       -4     
==========================================
- Hits        41912    41910       -2     
  Misses       5059     5059              
  Partials       49       49              

☔ View full report in Codecov by Sentry.
📢 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 merged commit f424e4e into main Jan 8, 2026
11 checks passed
@DylanPiercey DylanPiercey deleted the fix-scope-read-section branch January 8, 2026 16:38
@github-actions github-actions bot mentioned this pull request Jan 8, 2026
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.

2 participants