Skip to content

Conversation

@dmitryuk
Copy link

@dmitryuk dmitryuk commented Dec 6, 2024

Fixes hydrateFragment method throws hydration mismatch error despite of data-allow-mismatch attribute exists in parent node.

fragment not enough children test was wrong since Hydration completed but contains mismatches. error was ignored in test environment and not checked at all

__TEST__ flag prevented to catch error for testing

added toHaveBeenErrored method that looks same as toHaveBeenWarned, but error level

Summary by CodeRabbit

  • Tests

    • Enhanced hydration mismatch scenario test coverage with improved error assertions.
    • Added custom error assertion matcher for testing utilities.
  • Chores

    • Improved hydration error logging to conditionally suppress messages when appropriate.
    • Updated test infrastructure for better error tracking and validation.

✏️ Tip: You can customize this high-level summary in your review settings.

@dmitryuk dmitryuk force-pushed the fix/hydration-error-allow-mismatch branch from 51f813c to ab617d5 Compare December 6, 2024 09:26
@github-actions
Copy link

github-actions bot commented Dec 9, 2024

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 100 kB (+9 B) 37.9 kB (-1 B) 34.2 kB (+22 B)
vue.global.prod.js 158 kB (+9 B) 57.8 kB (+1 B) 51.5 kB (+87 B)

Usages

Name Size Gzip Brotli
createApp (CAPI only) 46.6 kB 18.3 kB 16.7 kB
createApp 54.6 kB 21.3 kB 19.4 kB
createSSRApp 58.7 kB (+9 B) 23 kB (+4 B) 20.9 kB (-2 B)
defineCustomElement 59.4 kB 22.8 kB 20.8 kB
overall 68.4 kB 26.4 kB 24 kB

@pkg-pr-new
Copy link

pkg-pr-new bot commented Dec 9, 2024

Open in Stackblitz

@vue/compiler-core

npm i https://pkg.pr.new/@vue/compiler-core@12505

@vue/compiler-dom

npm i https://pkg.pr.new/@vue/compiler-dom@12505

@vue/compiler-ssr

npm i https://pkg.pr.new/@vue/compiler-ssr@12505

@vue/compiler-sfc

npm i https://pkg.pr.new/@vue/compiler-sfc@12505

@vue/reactivity

npm i https://pkg.pr.new/@vue/reactivity@12505

@vue/runtime-core

npm i https://pkg.pr.new/@vue/runtime-core@12505

@vue/runtime-dom

npm i https://pkg.pr.new/@vue/runtime-dom@12505

@vue/server-renderer

npm i https://pkg.pr.new/@vue/server-renderer@12505

@vue/shared

npm i https://pkg.pr.new/@vue/shared@12505

vue

npm i https://pkg.pr.new/vue@12505

@vue/compat

npm i https://pkg.pr.new/@vue/compat@12505

commit: 9e90946

@edison1105
Copy link
Member

Thanks for the PR. I've made some minor tweaks 9e90946
LGTM

@edison1105 edison1105 added ready to merge The PR is ready to be merged. 🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. labels Dec 9, 2024
@dmitryuk
Copy link
Author

dmitryuk commented Dec 9, 2024

@edison1105 thanks you so much for instant help!

I very hope the PR will be merged next release

@dmitryuk
Copy link
Author

@edison1105 can you merge this PR, please?

@coderabbitai
Copy link

coderabbitai bot commented Dec 8, 2025

Walkthrough

The pull request refines hydration error handling by removing test-mode special treatment from mismatch logging, conditionally suppressing mismatch errors when children mismatches are explicitly allowed, and introducing a custom Vitest matcher (toHaveBeenErrored) to assert expected console.error calls during tests.

Changes

Cohort / File(s) Change Summary
Hydration source logic
packages/runtime-core/src/hydration.ts
Removed __TEST__ conditional from logMismatchError, making error suppression rely solely on the flag. Made logMismatchError() call in hydrateFragment conditional on isMismatchAllowed(container, MismatchTypes.CHILDREN) to suppress logging when children mismatches are permitted.
Hydration test assertions
packages/runtime-core/__tests__/hydration.spec.ts
Added test assertions for hydration error cases: expects error when Teleport unmount has mismatch, and ensures no error is raised for fragment-not-enough-children scenario under data-allow-mismatch.
Test infrastructure
scripts/setup-vitest.ts
Introduced custom Vitest matcher toHaveBeenErrored() to assert console.error calls; tracks asserted messages and throws if non-asserted errors remain after tests. Mirrors existing warn mock behavior for error handling.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • hydration.ts: Verify conditional logic in hydrateFragment correctly gates mismatch suppression and that removal of __TEST__ doesn't break test expectations
  • setup-vitest.ts: Review custom matcher implementation for proper error tracking, message aggregation, and afterEach hook behavior to ensure robust test assertion
  • hydration.spec.ts: Confirm test assertions align with source logic changes and properly exercise the conditional paths

Suggested labels

hammer: p3-minor-bug, scope:hydration

Suggested reviewers

  • johnsoncodehk
  • Doctor-wu
  • LittleSound

Poem

🐰 Hydration flows with grace so true,
Errors logged only when they're due,
Mismatches hushed by permission's call,
Test matchers catch them one and all! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main fix: preventing hydration errors when data-allow-mismatch is provided for fragments, which directly aligns with the core changes in hydration.ts.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
scripts/setup-vitest.ts (1)

8-13: Add toHaveBeenErrored to CustomMatchers interface with correct signature

The toHaveBeenErrored(): R declaration is missing the received parameter. According to Vitest's custom matcher API, matchers are functions called with (received, ...expected) where the first argument is the value passed to expect(...). The correct TypeScript signature should be toHaveBeenErrored(this: CustomMatcherContext): R or similar to match the documented Vitest pattern. Update the interface to properly reflect that the matcher receives the received value as its implicit context.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f51d3e2 and aa53bb5.

📒 Files selected for processing (3)
  • packages/runtime-core/__tests__/hydration.spec.ts (2 hunks)
  • packages/runtime-core/src/hydration.ts (2 hunks)
  • scripts/setup-vitest.ts (4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/setup-vitest.ts (1)
packages/compiler-sfc/src/script/context.ts (1)
  • error (146-150)
🔇 Additional comments (7)
packages/runtime-core/src/hydration.ts (1)

58-66: Expose hydration mismatch errors in tests while keeping noise minimal

Removing the __TEST__ short‑circuit and relying solely on hasLoggedMismatchError means the production‑level error message is now visible under test, but still only emitted once per process. That lines up nicely with the new toHaveBeenErrored matcher and keeps test output controlled.

packages/runtime-core/__tests__/hydration.spec.ts (2)

679-680: Assert that Teleport unmount mismatches surface as an error

This expectation correctly ties the Teleport unmount mismatch to the global "Hydration completed but contains mismatches." error, matching the new logging semantics and ensuring the regression that originally hid this signal in tests can’t come back.


2475-2478: Confirm data-allow-mismatch="children" suppresses the global error for fragment under-hydration

This negative toHaveBeenErrored assertion nicely covers the fragment “not enough children” case and verifies that, with data-allow-mismatch="children" on the container, we no longer surface the global hydration‑mismatch error even though the fragment structure is being fixed up.

scripts/setup-vitest.ts (4)

103-105: Track a dedicated console.error spy alongside console.warn

Declaring let error: MockInstance next to warn keeps the spying symmetrical and makes the new matcher and cleanup logic straightforward.


107-114: Spy on and stub console.error in beforeEach

Initializing error = vi.spyOn(console, 'error') and no-oping its implementation ensures all error logs are captured and won't pollute test output, while still being fully observable via the matcher and afterEach checks. Vitest guarantees beforeEach runs before every test case, so the spy is always initialized before any test logic executes.


135-149: Enforce assertion of console.error calls after each test

The nonAssertedErrors filter and subsequent throw mirror the existing warning behavior and enforce test hygiene by surfacing any unexpected console.error usage as a test failure. This is a recognized best practice in Vitest/Jest test setups—failing on unasserted console calls makes stray errors explicit rather than noisy and ensures tests remain deterministic.

Note this assumes the first argument to console.error is a string (same assumption as for warnings); any tests logging non-string values directly may need to be updated to wrap them in a message string.


18-37: New toHaveBeenErrored matcher correctly implements Vitest custom matcher signature

The matcher properly receives received: string from expect(), searches error.mock.calls, marks the substring as asserted, and returns the correct ExpectationResult shape with pass boolean and message function. The implementation follows the same pattern as toHaveBeenWarned and is appropriate for asserting on specific error messages and catching unasserted errors in afterEach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. ready to merge The PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants