feat: optimise input.content serialisation#3184
Open
vwong wants to merge 4 commits into
Open
Conversation
🦋 Changeset detectedLatest commit: c84c9b1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
Contributor
Author
|
I'm terrible at finding names, let me know if you want to change the name of the fixture to match existing conventions. |
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis PR makes subscription writes conditional by widening 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/runtime-tags/src/translator/util/signals.ts (1)
1091-1112: 💤 Low valueLogic looks correct; consider deduplicating the section-reason computation.
The case analysis is sound: only wrap
identifierwithgetExprIfSerializedwhen the closure-scopes reason is dynamic and not the same condition as the section's effective reason — otherwise the subscribers Set is guaranteed to be serialized in lock-step with the_subscribecall site (or always present), so passing the bare identifier is safe.Minor cleanup:
effectiveSectionReasonhere is identical tosectionSerializeReasoncomputed below at lines 1137-1139. It also doesn't depend on the closure, so it can be hoisted out of theforEachoversection.referencedClosuresand reused in both places to make the "is this section being serialized" notion single-sourced.♻️ Suggested refactor
const body = path.node.body as t.Statement[]; const allSignals = Array.from(getSignals(section).values()); const scopeIdIdentifier = getScopeIdIdentifier(section); + const sectionSerializeReason = nonAnalyzedForceSerializedSection.has(section) + ? true + : section.serializeReason; forEach(section.referencedClosures, (closure) => { ... - } else { - const closureScopesReason = getSerializeReason( - closure.section, - closure, - getAccessorPrefix().ClosureScopes, - ); - const effectiveSectionReason = nonAnalyzedForceSerializedSection.has( - section, - ) - ? true - : section.serializeReason; - const subscribeArg = - isReasonDynamic(closureScopesReason) && - !isSameReason(closureScopesReason, effectiveSectionReason) + } else { + const closureScopesReason = getSerializeReason( + closure.section, + closure, + getAccessorPrefix().ClosureScopes, + ); + const subscribeArg = + isReasonDynamic(closureScopesReason) && + !isSameReason(closureScopesReason, sectionSerializeReason) ? getExprIfSerialized( closure.section, closureScopesReason, identifier, ) : identifier; ... - const sectionSerializeReason = nonAnalyzedForceSerializedSection.has(section) - ? true - : section.serializeReason;🤖 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 1091 - 1112, Hoist the section-level serialization check into a single variable (e.g., sectionSerializeReason) computed once using getSerializeReason/nonAnalyzedForceSerializedSection logic instead of recomputing effectiveSectionReason inside the referencedClosures loop; then use that sectionSerializeReason in the closure loop where closureScopesReason, subscribeArg, and getExprIfSerialized are determined and also reuse it where sectionSerializeReason is computed later (previously lines computing sectionSerializeReason), so the "is this section being serialized" logic is single-sourced and shared between addWriteScopeBuilder calls and the subsequent code that currently duplicates the computation.packages/runtime-tags/src/html/writer.ts (1)
1569-1577: 💤 Low valueType for
subscribersis inconsistent with writer.ts conventions.Elsewhere in this file, optional/conditional flags use
0 | 1(e.g._attr_content'sserializeReason?: 1 | 0,_sep,_el_resume, the_for_*serialize*params) orundefined | 1(e.g._serialize_if,_serialize_guard). The parameter's runtime value comes from translator-generated code that either passes aSetdirectly or wraps it viagetExprIfSerialized, which returns theSetor a falsy value (specificallyundefinedin static paths). The declared type usesfalse, which is not a falsy marker in this codebase's serialization conventions.Tighten the union to
Set<ScopeInternals> | undefinedto match the actual values emitted from the translator.🤖 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/html/writer.ts` around lines 1569 - 1577, The _subscribe function's subscribers parameter is typed incorrectly as `Set<ScopeInternals> | false`; change it to `Set<ScopeInternals> | undefined` to match translator/runtime conventions (translator uses `getExprIfSerialized` which yields a Set or undefined). Update the function signature for `_subscribe(subscribers: Set<ScopeInternals> | undefined, scope: ScopeInternals)` and keep the existing runtime check and calls to `$chunk.boundary.state.serializer.writeCall(scope, subscribers, "add")` and `referenceScope(scope)` unchanged.
🤖 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.
Nitpick comments:
In `@packages/runtime-tags/src/html/writer.ts`:
- Around line 1569-1577: The _subscribe function's subscribers parameter is
typed incorrectly as `Set<ScopeInternals> | false`; change it to
`Set<ScopeInternals> | undefined` to match translator/runtime conventions
(translator uses `getExprIfSerialized` which yields a Set or undefined). Update
the function signature for `_subscribe(subscribers: Set<ScopeInternals> |
undefined, scope: ScopeInternals)` and keep the existing runtime check and calls
to `$chunk.boundary.state.serializer.writeCall(scope, subscribers, "add")` and
`referenceScope(scope)` unchanged.
In `@packages/runtime-tags/src/translator/util/signals.ts`:
- Around line 1091-1112: Hoist the section-level serialization check into a
single variable (e.g., sectionSerializeReason) computed once using
getSerializeReason/nonAnalyzedForceSerializedSection logic instead of
recomputing effectiveSectionReason inside the referencedClosures loop; then use
that sectionSerializeReason in the closure loop where closureScopesReason,
subscribeArg, and getExprIfSerialized are determined and also reuse it where
sectionSerializeReason is computed later (previously lines computing
sectionSerializeReason), so the "is this section being serialized" logic is
single-sourced and shared between addWriteScopeBuilder calls and the subsequent
code that currently duplicates the computation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39b7a834-5380-46b6-842b-1cde377c0288
⛔ Files ignored due to path filters (21)
packages/runtime-tags/src/__tests__/fixtures/basic-nested-params/__snapshots__/html.expected/template.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/dynamic-closure-with-function/__snapshots__/html.expected/template.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/hoist-only/__snapshots__/html.expected/template.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/hoist-only/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/input-missing-property/__snapshots__/html.expected/template.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/namespaced-tags/__snapshots__/html.expected/template.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/namespaced-tags/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/spread-to-known-rest-input-with-attr-tags-deopt/__snapshots__/html.expected/tags/wrap.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/spread-to-known-rest-input-with-attr-tags-deopt/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/static-content/__snapshots__/csr-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/static-content/__snapshots__/csr.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/static-content/__snapshots__/dom.expected/tags/inner.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/static-content/__snapshots__/dom.expected/tags/outer.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/static-content/__snapshots__/dom.expected/template.hydrate.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/static-content/__snapshots__/dom.expected/template.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/static-content/__snapshots__/html.expected/tags/inner.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/static-content/__snapshots__/html.expected/tags/outer.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/static-content/__snapshots__/html.expected/template.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/static-content/__snapshots__/resume-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/static-content/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/toggle-nested/__snapshots__/html.expected/template.jsis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (7)
.changeset/strong-symbols-try.mdpackages/runtime-tags/src/__tests__/fixtures/static-content/tags/inner.markopackages/runtime-tags/src/__tests__/fixtures/static-content/tags/outer.markopackages/runtime-tags/src/__tests__/fixtures/static-content/template.markopackages/runtime-tags/src/__tests__/fixtures/static-content/test.tspackages/runtime-tags/src/html/writer.tspackages/runtime-tags/src/translator/util/signals.ts
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3184 +/- ##
=======================================
Coverage 92.75% 92.75%
=======================================
Files 371 371
Lines 47296 47318 +22
Branches 3376 3380 +4
=======================================
+ Hits 43870 43891 +21
- Misses 3394 3395 +1
Partials 32 32 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/runtime-tags/src/__tests__/fixtures/content-with-state/test.ts (1)
3-8: ⚡ Quick winPlace exported
configbefore helper declarations to match test file structure guidelines.This file currently defines a helper before the public export. Reordering to put
export const configfirst (while keepingclickas a hoisted function declaration) aligns with the expected top-down layout.Proposed refactor
import type { TestConfig } from "../../main.test"; -function click(container: Element) { - container.querySelector<HTMLButtonElement>("#increment")!.click(); -} - export const config: TestConfig = { steps: [{}, click, click, click], }; + +function click(container: Element) { + container.querySelector<HTMLButtonElement>("#increment")!.click(); +}As per coding guidelines,
**/*.{js,ts,jsx,tsx}files should be organized from “Public API (exports)” to helpers and use hoisting for top-down structure.🤖 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/__tests__/fixtures/content-with-state/test.ts` around lines 3 - 8, Move the exported TestConfig declaration to the top so the public API appears before helpers: place "export const config: TestConfig = { steps: [{}, click, click, click] }" before the helper declaration, keeping the "function click(container: Element) { ... }" as a hoisted function declaration below; ensure references to "click" remain unchanged and the file still exports the same symbol order.
🤖 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.
Nitpick comments:
In `@packages/runtime-tags/src/__tests__/fixtures/content-with-state/test.ts`:
- Around line 3-8: Move the exported TestConfig declaration to the top so the
public API appears before helpers: place "export const config: TestConfig = {
steps: [{}, click, click, click] }" before the helper declaration, keeping the
"function click(container: Element) { ... }" as a hoisted function declaration
below; ensure references to "click" remain unchanged and the file still exports
the same symbol order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d423789-6b1e-4b02-a699-40b28a03d4d8
⛔ Files ignored due to path filters (11)
packages/runtime-tags/src/__tests__/fixtures/content-with-state/__snapshots__/csr-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/content-with-state/__snapshots__/csr.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/content-with-state/__snapshots__/dom.expected/tags/inner.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/content-with-state/__snapshots__/dom.expected/tags/outer.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/content-with-state/__snapshots__/dom.expected/template.hydrate.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/content-with-state/__snapshots__/dom.expected/template.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/content-with-state/__snapshots__/html.expected/tags/inner.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/content-with-state/__snapshots__/html.expected/tags/outer.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/content-with-state/__snapshots__/html.expected/template.jsis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/content-with-state/__snapshots__/resume-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**packages/runtime-tags/src/__tests__/fixtures/content-with-state/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (4)
packages/runtime-tags/src/__tests__/fixtures/content-with-state/tags/inner.markopackages/runtime-tags/src/__tests__/fixtures/content-with-state/tags/outer.markopackages/runtime-tags/src/__tests__/fixtures/content-with-state/template.markopackages/runtime-tags/src/__tests__/fixtures/content-with-state/test.ts
✅ Files skipped from review due to trivial changes (3)
- packages/runtime-tags/src/tests/fixtures/content-with-state/template.marko
- packages/runtime-tags/src/tests/fixtures/content-with-state/tags/outer.marko
- packages/runtime-tags/src/tests/fixtures/content-with-state/tags/inner.marko
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
In many cases, we want to pass static content from grandparent to parent to child, even if the intermediate parent may have event handlers (thus state-ful). This PR introduces an optimisation that avoids serialising code to generate the otherwise static content..
Checklist: