-
Notifications
You must be signed in to change notification settings - Fork 655
fix(dynamic-tag): stricter normalization #2868
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: 7ff9e1b 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 |
WalkthroughUpdated normalization for dynamic renderers (packages/runtime-tags/src/common/helpers.ts and .sizes/dom.js): strings are returned directly; non-string objects are normalized to value.content || value.default || value and only returned if they contain an ___id, otherwise undefined. Added test fixtures and snapshots for dynamic-tag-content-default (Marko templates, tags/child.marko, .name-cache.json, DOM/HTML/SSR/CSR/resume snapshots). Adjusted size metadata in .sizes.json and related comments. No exported/public API signatures changed. Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/ssr.expected.md (1)
20-20
: Minor: Add language identifier to fenced code block.The Mutations section has a fenced code block without a language identifier. Add
text
or another appropriate identifier to satisfy markdown linting.Apply this diff:
# Mutations -``` +```text INSERT htmlBased on static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.sizes.json
(1 hunks).sizes/dom.js
(2 hunks)packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/.name-cache.json
(1 hunks)packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/csr-sanitized.expected.md
(1 hunks)packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/csr.expected.md
(1 hunks)packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/dom.expected/tags/child.js
(1 hunks)packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/dom.expected/template.hydrate.js
(1 hunks)packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/dom.expected/template.js
(1 hunks)packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/html.expected/tags/child.js
(1 hunks)packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/html.expected/template.js
(1 hunks)packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/resume-sanitized.expected.md
(1 hunks)packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/resume.expected.md
(1 hunks)packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/ssr-sanitized.expected.md
(1 hunks)packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/ssr.expected.md
(1 hunks)packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/tags/child.marko
(1 hunks)packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/template.marko
(1 hunks)packages/runtime-tags/src/common/helpers.ts
(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/html.expected/tags/child.js (1)
packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/html.expected/template.js (1)
$scope0_id
(4-4)
packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/dom.expected/tags/child.js (1)
packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/dom.expected/template.js (5)
$template
(1-1)$template
(1-1)$walks
(2-2)$walks
(2-2)$setup
(6-13)
packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/dom.expected/template.js (1)
packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/dom.expected/tags/child.js (6)
$template
(1-1)$template
(1-1)$walks
(2-2)$walks
(2-2)$setup
(3-3)$setup
(3-3)
packages/runtime-tags/src/common/helpers.ts (1)
.sizes/dom.js (3)
value
(138-141)value
(1029-1029)normalized
(74-74)
packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/html.expected/template.js (2)
packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/html.expected/tags/child.js (1)
$scope0_id
(4-4)packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-downstream-effect/__snapshots__/html.expected/template.js (1)
$scope1_id
(7-7)
🪛 markdownlint-cli2 (0.18.1)
packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/ssr.expected.md
20-20: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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@18
- GitHub Check: test: node@20
- GitHub Check: test: node@22
🔇 Additional comments (11)
.sizes.json (1)
10-11
: Size changes look reasonable for the fix.The bundle size increased by 52 bytes (min) but improved brotli compression by 8 bytes, which is expected for adding stricter normalization checks.
packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/tags/child.marko (1)
1-2
: Test fixture looks good.This fixture correctly tests dynamic content rendering with
${input}
, which will exercise the normalization logic when the parent passes content (or not).packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/template.marko (1)
1-2
: Test coverage is appropriate.This fixture correctly tests both scenarios:
- Line 1: Self-closing
<child/>
(the error case mentioned in PR objectives)- Line 2:
<child>Hello</child>
(the working case)This ensures the fix handles both cases properly.
packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/csr.expected.md (1)
1-12
: CSR snapshot shows correct behavior after fix.The snapshot confirms that both the self-closing
<child/>
and<child>Hello</child>
render correctly, producing an empty<div />
and<div>Hello</div>
respectively. The mutations show both elements are properly inserted.packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/csr-sanitized.expected.md (1)
1-7
: Sanitized CSR snapshot is consistent.The rendered output matches the full CSR snapshot, confirming consistent behavior.
packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/.name-cache.json (1)
1-5
: Standard test metadata.This is a standard name-cache file for test fixtures with no concerns.
packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/resume-sanitized.expected.md (1)
1-7
: Resume snapshot confirms hydration compatibility.The output matches CSR snapshots, ensuring the fix works correctly in hydration scenarios.
packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/resume.expected.md (1)
1-12
: Resume snapshot shows complete server-rendered output.The full HTML document structure is correct, with both divs rendering as expected in the body.
Critical: Core implementation files are missing from this review.
The AI summary and PR objectives mention changes to
packages/runtime-tags/src/common/helpers.ts
and.sizes/dom.js
that implement the actual normalization fix. These files were not provided for review, making it impossible to assess:
- The correctness of the normalization logic
- Potential edge cases or bugs in the implementation
- Whether the fix properly handles all input types (string, object with ___id, object without ___id, null/undefined)
Please ensure these critical implementation files are included in the review for a complete assessment of the fix.
packages/runtime-tags/src/common/helpers.ts (1)
70-76
: LGTM! Stricter normalization prevents errors with missing content.The updated logic correctly addresses the issue where
<${input}/>
would fail wheninput.content
doesn't exist. By returningundefined
when the normalized value lacks an___id
property, invalid objects are filtered out.Minor note: The
||
operator treats falsy values (0, false, "") as absent. Ifcontent: 0
orcontent: false
are valid in some scenarios, consider using nullish coalescing (??
) instead:-const normalized = value.content || value.default || value; +const normalized = value.content ?? value.default ?? value;However, given that content is typically a renderer object or string in Marko templates, the current approach is likely sufficient.
.sizes/dom.js (1)
71-77
: LGTM! Compiled version matches the TypeScript implementation.The normalization logic correctly mirrors the changes in
packages/runtime-tags/src/common/helpers.ts
, returningundefined
when the normalized value lacks an___id
property.packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-content-default/__snapshots__/dom.expected/tags/child.js (1)
1-7
: LGTM! Snapshot file looks correct for the test fixture.This snapshot represents the expected compiler output for a child component using the dynamic-tag shorthand (
<${input}/>
). The structure follows standard Marko runtime-tags patterns:
- Lines 1-3 define the template metadata ($template, $walks, $setup)
- Line 6 uses the
_._const("input", $dynamicTag)
pattern to handle input, which should work correctly with the stricter normalization changes mentioned in the PRAs this is a snapshot file (expected compiler output), ensure the tests pass to verify this snapshot matches the actual compiler output after the normalization changes in
helpers.ts
.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2868 +/- ##
==========================================
- Coverage 88.07% 88.01% -0.06%
==========================================
Files 368 368
Lines 45885 45891 +6
Branches 3666 3668 +2
==========================================
- Hits 40414 40392 -22
- Misses 5439 5467 +28
Partials 32 32 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Before this fix, if
input.content
didn't exist then the<${input}/>
shorthand led to an error: