Skip to content

test(ssr): add SSR and hydration tests for 'getServerSnapshot'#1209

Draft
sukvvon wants to merge 4 commits intopmndrs:mainfrom
sukvvon:test/basic-getServerSnapshot
Draft

test(ssr): add SSR and hydration tests for 'getServerSnapshot'#1209
sukvvon wants to merge 4 commits intopmndrs:mainfrom
sukvvon:test/basic-getServerSnapshot

Conversation

@sukvvon
Copy link
Contributor

@sukvvon sukvvon commented Mar 7, 2026

Summary

  • Add ssr.test.tsx with tests for getServerSnapshot callback in useSnapshot
  • Test initial snapshot and latest snapshot after proxy state changes
  • Add hydration mismatch test to verify getServerSnapshot prevents hydration error
  • Covers previously uncovered line 161 in src/react.ts

Check List

  • pnpm run fix for formatting and linting code and docs

@vercel
Copy link

vercel bot commented Mar 7, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
valtio Ready Ready Preview, Comment Mar 8, 2026 3:35am

Request Review

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 7, 2026

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Mar 7, 2026

More templates

npm i https://pkg.pr.new/valtio@1209

commit: 6ee03db

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

Unless I don't misunderstand, this is not a valid test.

)

expect(view).toContain('count:')
expect(view).toContain('0')
Copy link
Member

Choose a reason for hiding this comment

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

I think this test still passes even if we remove getServerSnapshot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dai-shi I verified by removing getServerSnapshot — the test fails with Missing getServerSnapshot, which is required for server-rendered content. So renderToString does invoke getServerSnapshot, and this test validates its presence.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, but this test doesn't cover the behavior of getServerSnapshot. For example, if we have the getServerSnapshot implementation identical to that of getSnapshot, the test may pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dai-shi You're right — this test doesn't distinguish the behavior of getServerSnapshot from getSnapshot. However, it serves as a regression test to ensure getServerSnapshot is provided, since renderToString throws if it's missing. I also added a test (3aea407) that verifies the latest snapshot is returned after proxy state changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dai-shi Added a hydration mismatch test (a5c4d42) that verifies getServerSnapshot prevents hydration error by rendering with renderToString and hydrating with hydrateRoot, asserting no console.error is called.

@sukvvon sukvvon changed the title test(basic): add test for 'getServerSnapshot' with 'renderToString' test(ssr): add test for 'getServerSnapshot' with 'renderToString' Mar 7, 2026
@sukvvon sukvvon requested a review from dai-shi March 7, 2026 14:18
@sukvvon sukvvon changed the title test(ssr): add test for 'getServerSnapshot' with 'renderToString' test(ssr): add tests for 'getServerSnapshot' in SSR Mar 7, 2026
Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

I think I've got a better explanation.

The purpose of getServerSnapshot is to avoid hydration error.
So, the test should cover that. Testing if getServerSnapshot is defined is too trivial (only to increase the test coverage without much benefit.)

We've never tested if the current implementation can avoid hydration error.

() => snapshot(proxyObject),

If the test reveals it, we need to fix the implementation.

@sukvvon sukvvon changed the title test(ssr): add tests for 'getServerSnapshot' in SSR test(ssr): add SSR and hydration tests for 'getServerSnapshot' Mar 8, 2026
@sukvvon sukvvon requested a review from dai-shi March 8, 2026 03:42
expect(view).toContain('5')
})

it('should not cause hydration mismatch', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

This is too naive. The AI doesn't understand the underlying problem.
Try to create a bug so that such a hydration-mismatch test fails.
I'm not sure if it's possible. But, otherwise, it's not valid for getServerSnapshot test.

@sukvvon sukvvon marked this pull request as draft March 8, 2026 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants