fix(hydration): respect data-allow-mismatch on conditional branches#12801
Conversation
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
skirtles-code
left a comment
There was a problem hiding this comment.
It seems like this adds a new type of allowable mismatch. Is there any precedent for this type of mismatch?
If we want to allow this then we may need to consider similar cases, like:
Those seem essentially the same to me.
| node: Node, | ||
| { props }: VNode, | ||
| ): boolean { | ||
| return isComment(node) && props != null && hasOwn(props, allowMismatchAttr) |
There was a problem hiding this comment.
If a node has data-allow-mismatch="class" then I wouldn't expect it to suppress hydration errors for unexpected comment nodes.
There was a problem hiding this comment.
Fixed by making this a children-scoped node mismatch allowance instead of checking only for the presence of data-allow-mismatch on the client vnode.
The updated logic now checks the parent/server node/client vnode through the same data-allow-mismatch type parsing, so only an empty value or children suppresses node mismatches. class/style/attribute no longer suppress them.
I also added coverage for the flipped v-if case, the v-else branch case, and a data-allow-mismatch="class" negative case.
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughHydration mismatch handling now checks node, parent, and vnode ChangesHydration mismatch allowance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/runtime-core/__tests__/hydration.spec.ts (1)
2302-2310: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a positive
childrentoken case for vnode-side allowance.This test covers the empty-attribute fast path, but the new vnode-side helper also relies on parsing
data-allow-mismatch="children". Adding that variant would lock down the documented non-empty token behavior for comment-basedv-ifmismatches.🤖 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-core/__tests__/hydration.spec.ts` around lines 2302 - 2310, Add a positive vnode-side token test for the comment mismatch case in hydration.spec.ts, since the current `test('comment mismatch (v-if)')` only covers the empty `data-allow-mismatch` fast path. Extend the existing `mountWithHydration`/`expect(container.innerHTML)` coverage to include `data-allow-mismatch="children"` on the vnode and assert the same no-warning behavior, so the `data-allow-mismatch` parsing in the vnode-side helper is covered for non-empty tokens too.
🤖 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-core/__tests__/hydration.spec.ts`:
- Around line 2302-2310: Add a positive vnode-side token test for the comment
mismatch case in hydration.spec.ts, since the current `test('comment mismatch
(v-if)')` only covers the empty `data-allow-mismatch` fast path. Extend the
existing `mountWithHydration`/`expect(container.innerHTML)` coverage to include
`data-allow-mismatch="children"` on the vnode and assert the same no-warning
behavior, so the `data-allow-mismatch` parsing in the vnode-side helper is
covered for non-empty tokens too.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b102bf87-b49d-4ab9-aa32-47377be7c70f
📒 Files selected for processing (2)
packages/runtime-core/__tests__/hydration.spec.tspackages/runtime-core/src/hydration.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/runtime-core/src/hydration.ts`:
- Around line 404-406: The hydration mismatch check is skipping empty
pre-transition class state because hydration.ts only caches and reads truthy
class values in the Transition flow. Update the logic around content.$cls and
the later propHasMismatch() class comparison so empty string / missing server
class is preserved explicitly for Transition.appear, allowing beforeEnter()
temporary classes to be excluded from mismatch detection. Use the existing
hydration transition hooks paths and the class caching/reading logic in the
relevant hydration helpers to keep the pre-transition class snapshot intact even
when it is empty.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55e6ffda-3f5e-4262-ba9c-856d06b248c2
📒 Files selected for processing (2)
packages/runtime-core/__tests__/hydration.spec.tspackages/runtime-core/src/hydration.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/runtime-core/src/hydration.ts`:
- Around line 404-406: The hydration mismatch check is skipping empty
pre-transition class state because hydration.ts only caches and reads truthy
class values in the Transition flow. Update the logic around content.$cls and
the later propHasMismatch() class comparison so empty string / missing server
class is preserved explicitly for Transition.appear, allowing beforeEnter()
temporary classes to be excluded from mismatch detection. Use the existing
hydration transition hooks paths and the class caching/reading logic in the
relevant hydration helpers to keep the pre-transition class snapshot intact even
when it is empty.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55e6ffda-3f5e-4262-ba9c-856d06b248c2
📒 Files selected for processing (2)
packages/runtime-core/__tests__/hydration.spec.tspackages/runtime-core/src/hydration.ts
🛑 Comments failed to post (1)
packages/runtime-core/src/hydration.ts (1)
404-406: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve empty pre-transition classes when checking hydration mismatch.
Line 406 only caches truthy class values, and Line 800 only reads truthy cached values. For
<Transition appear>with aclassprop that hydrates fromclass=""or no server class,beforeEnter()can add temporary transition classes beforepropHasMismatch(), producing a false class mismatch.Proposed fix
if (needCallTransitionHooks) { const cls = content.getAttribute('class') - if (cls) content.$cls = cls + if (props && 'class' in props) content.$cls = cls || '' transition!.beforeEnter(content) }- if (el.$cls) { + if ('$cls' in el) { actual = el.$cls delete el.$cls } else {Also applies to: 800-804
🤖 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-core/src/hydration.ts` around lines 404 - 406, The hydration mismatch check is skipping empty pre-transition class state because hydration.ts only caches and reads truthy class values in the Transition flow. Update the logic around content.$cls and the later propHasMismatch() class comparison so empty string / missing server class is preserved explicitly for Transition.appear, allowing beforeEnter() temporary classes to be excluded from mismatch detection. Use the existing hydration transition hooks paths and the class caching/reading logic in the relevant hydration helpers to keep the pre-transition class snapshot intact even when it is empty.
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
close #12782
Summary by CodeRabbit
Bug Fixes
data-allow-mismatchis respected more consistently for nested and conditional/branch-based rendering, including cases involving comment nodes.Tests