-
Notifications
You must be signed in to change notification settings - Fork 664
fix: store section with scope reads #3032
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 7acdc2c 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 |
WalkthroughThe changes fix a bug in the Marko compiler where section information was not stored during scope reads. A new changeset documents the patch for 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this 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
Signalfrom a type to an interface and using concrete boolean fields (hasDynamicSubscribers,hasSideEffect) instead of optional fields improves type safety and code clarity. The explicitundefinedunions forextraArgs,prependStatements, andbuildAssignmentmake the API contract clearer.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-and-update/__snapshots__/.name-cache.jsonis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-and-update/__snapshots__/csr-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-and-update/__snapshots__/csr.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-and-update/__snapshots__/dom.expected/template.hydrate.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-and-update/__snapshots__/dom.expected/template.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-and-update/__snapshots__/html.expected/template.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-and-update/__snapshots__/resume-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-and-update/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-and-update/__snapshots__/ssr-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-and-update/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (5)
.changeset/eager-rabbits-live.mdpackages/runtime-tags/src/__tests__/fixtures/native-tag-ref-and-update/template.markopackages/runtime-tags/src/translator/util/references.tspackages/runtime-tags/src/translator/util/scope-read.tspackages/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-tagsand 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
refExtravariable and explicit assignment ofrefExtra.section = refSectionensures 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.sectionsimplifies the calling convention and makes the typical case (where section matches reference.section) more explicit. This reduces the need for callers to passundefined.
90-94: LGTM! Explicit section metadata attachment.The conditional attachment of
exprExtra.readandexprExtra.sectionensures that section information is properly stored when the reference is in the same section and is not a DOM binding. This complements the fix inreferences.tsand 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 (usinggetScopeExpression), 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
hasSideEffectbased 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 = truewhen 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 Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
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
hasSideEffectlogic a bit.