test(et): drop generic host render fallback#2552
Conversation
|
📝 WalkthroughWalkthroughThis PR enforces compiled Element Template paths for string host vnode rendering, removing fallback rendering for uncompiled vnodes and introducing dev-mode errors for invalid vnode shapes. Test expectations are updated to match primitive outputs and raw text rendering semantics. ChangesString Host VNode Rendering & Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Merging this PR will degrade performance by 14.67%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | 008-many-use-state-destroyBackground |
9.5 ms | 8 ms | +18.85% |
| ❌ | transform 1000 view elements |
40 ms | 46.9 ms | -14.67% |
Comparing Yradex:slice/element-template/01 (d46a9bc) with main (f706c3a)
Footnotes
-
26 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
React MTF Example#892 Bundle Size — 196.68KiB (0%).d46a9bc(current) vs f706c3a main#884(baseline) Bundle metrics
|
| Current #892 |
Baseline #884 |
|
|---|---|---|
0B |
0B |
|
0B |
0B |
|
0% |
0% |
|
0 |
0 |
|
3 |
3 |
|
174 |
174 |
|
66 |
66 |
|
44.05% |
44.05% |
|
2 |
2 |
|
0 |
0 |
Bundle size by type no changes
| Current #892 |
Baseline #884 |
|
|---|---|---|
111.23KiB |
111.23KiB |
|
85.45KiB |
85.45KiB |
Bundle analysis report Branch Yradex:slice/element-template/01 Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#7760 Bundle Size — 225.52KiB (0%).d46a9bc(current) vs f706c3a main#7752(baseline) Bundle metrics
|
| Current #7760 |
Baseline #7752 |
|
|---|---|---|
0B |
0B |
|
0B |
0B |
|
0% |
0% |
|
0 |
0 |
|
4 |
4 |
|
180 |
180 |
|
69 |
69 |
|
44.54% |
44.54% |
|
2 |
2 |
|
0 |
0 |
Bundle size by type no changes
| Current #7760 |
Baseline #7752 |
|
|---|---|---|
145.76KiB |
145.76KiB |
|
79.77KiB |
79.77KiB |
Bundle analysis report Branch Yradex:slice/element-template/01 Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#9333 Bundle Size — 900.03KiB (0%).d46a9bc(current) vs f706c3a main#9325(baseline) Bundle metrics
Bundle size by type
|
| Current #9333 |
Baseline #9325 |
|
|---|---|---|
495.9KiB |
495.9KiB |
|
401.92KiB |
401.92KiB |
|
2.22KiB |
2.22KiB |
Bundle analysis report Branch Yradex:slice/element-template/01 Project dashboard
Generated by RelativeCI Documentation Report issue
React External#875 Bundle Size — 680.82KiB (0%).d46a9bc(current) vs f706c3a main#867(baseline) Bundle metrics
|
| Current #875 |
Baseline #867 |
|
|---|---|---|
0B |
0B |
|
0B |
0B |
|
0% |
0% |
|
0 |
0 |
|
3 |
3 |
|
17 |
17 |
|
5 |
5 |
|
8.59% |
8.59% |
|
0 |
0 |
|
0 |
0 |
Bundle analysis report Branch Yradex:slice/element-template/01 Project dashboard
Generated by RelativeCI Documentation Report issue
React Example (Element Template)#10 Bundle Size — 198.09KiB (-0.26%).c986b9a(current) vs 7abb0a9 main#4(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch Yradex:slice/element-template/01 Project dashboard Generated by RelativeCI Documentation Report issue |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/react/runtime/src/element-template/runtime/render/render-to-opcodes.ts`:
- Around line 268-271: The new dev-only throws in render-to-opcodes (the checks
using isCompiledEtHostType in the main-thread renderer) happen after
_renderToString() has attached vnode[PARENT] and fired beforeDiff/beforeDiff2,
so ensure you call cleanupVNode(vnode) before throwing to run
afterDiff/unmount/parent reset; update both offending throw sites (the one
around the isCompiledEtHostType check at the shown location and the similar one
at the later 336–338 location) to invoke cleanupVNode(vnode) first (keeping the
throw behavior intact) so cleanup always runs on the fail-fast paths.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a56482b9-3f5d-41fc-b9f1-3b2ad83810bf
📒 Files selected for processing (2)
packages/react/runtime/__test__/element-template/runtime/render/render-to-opcodes.et.test.jsxpackages/react/runtime/src/element-template/runtime/render/render-to-opcodes.ts
React Example (Element Template)#22 Bundle Size — 198.09KiB (-0.26%).c986b9a(current) vs f706c3a main#20(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch Yradex:slice/element-template/01 Project dashboard Generated by RelativeCI Documentation Report issue |
React Example (Element Template)#23 Bundle Size — 198.09KiB (-0.26%).c986b9a(current) vs f706c3a main#20(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch Yradex:slice/element-template/01 Project dashboard Generated by RelativeCI Documentation Report issue |
aaff512 to
b92dac8
Compare
React Example (Element Template)#24 Bundle Size — 198.09KiB (-0.26%).b92dac8(current) vs f706c3a main#20(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch Yradex:slice/element-template/01 Project dashboard Generated by RelativeCI Documentation Report issue |
React Example (Element Template)#25 Bundle Size — 198.09KiB (-0.26%).b92dac8(current) vs f706c3a main#20(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch Yradex:slice/element-template/01 Project dashboard Generated by RelativeCI Documentation Report issue |
b92dac8 to
d46a9bc
Compare
React Example (Element Template)#26 Bundle Size — 198.09KiB (-0.26%).d46a9bc(current) vs f706c3a main#20(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch Yradex:slice/element-template/01 Project dashboard Generated by RelativeCI Documentation Report issue |
React Example (Element Template)#28 Bundle Size — 198.09KiB (-0.26%).d46a9bc(current) vs f706c3a main#20(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch Yradex:slice/element-template/01 Project dashboard Generated by RelativeCI Documentation Report issue |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d46a9bc721
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react/runtime/__test__/element-template/runtime/render/render-to-opcodes.et.test.jsx (1)
66-101: ⚡ Quick winGuard dev-only invariant tests with
__DEV__checks.These assertions target dev-only behavior, but they currently run unconditionally. Adding early guards in the tests at Line 66, Line 72, and Line 78 avoids environment-coupled failures if this suite is ever executed against a non-dev build.
Suggested patch
it('throws in development when a plain host vnode reaches the ET render path', () => { + if (!__DEV__) return; expect(() => renderToString(h('view', null))).toThrow( 'Element Template main-thread renderer received an uncompiled host vnode: view', ); }); it('throws in development when an invalid vnode reaches the ET render path', () => { + if (!__DEV__) return; expect(() => renderToString({ type: null, props: {} })).toThrow( 'Element Template main-thread renderer received an invalid vnode.', ); }); it('cleans vnodes before throwing development renderer invariant errors', () => { + if (!__DEV__) return; const previousDiffed = options[DIFFED]; const cleaned = [];🤖 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/react/runtime/__test__/element-template/runtime/render/render-to-opcodes.et.test.jsx` around lines 66 - 101, These test cases assert dev-only invariants and must be gated by the __DEV__ flag: wrap the three affected tests that call renderToString(h('view', ...)), renderToString({type:null,...}) and the "cleans vnodes..." test with early guards checking if (__DEV__) so they only run in development; update the three "it(...)" blocks in the file to return/skip when __DEV__ is false, preserving the existing use of renderToString, h, options[DIFFED], DIFFED and PARENT inside the guarded blocks so the behavior and cleanup assertions remain 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/react/runtime/__test__/element-template/runtime/render/render-to-opcodes.et.test.jsx`:
- Around line 66-101: These test cases assert dev-only invariants and must be
gated by the __DEV__ flag: wrap the three affected tests that call
renderToString(h('view', ...)), renderToString({type:null,...}) and the "cleans
vnodes..." test with early guards checking if (__DEV__) so they only run in
development; update the three "it(...)" blocks in the file to return/skip when
__DEV__ is false, preserving the existing use of renderToString, h,
options[DIFFED], DIFFED and PARENT inside the guarded blocks so the behavior and
cleanup assertions remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9441ea78-60b9-4193-9fd3-196d5cbdf931
📒 Files selected for processing (2)
packages/react/runtime/__test__/element-template/runtime/render/render-to-opcodes.et.test.jsxpackages/react/runtime/src/element-template/runtime/render/render-to-opcodes.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react/runtime/src/element-template/runtime/render/render-to-opcodes.ts
Summary by CodeRabbit
Tests
Bug Fixes
Overview
This removes the Element Template main-thread renderer generic host-vnode fallback. The renderer now accepts compiled ET host nodes only; development builds fail fast when a plain host vnode reaches this path, while production keeps the framework-internal invariant out of the hot path.
Key Points
render-to-opcodes._et_*, dynamic-entry_et_*, and raw-text template hosts.viewortexthosts.Validation
pnpm run test:etpnpm testChecklist