Skip to content
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

[test] Replace createClientRender with new createRenderer API #29471

Merged
merged 3 commits into from
Nov 4, 2021

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Nov 2, 2021

TL;DR:

-const render = createClientRender();
+const { render } = createRenderer();

First commit holds most of the work. Subsequent commits are copy&paste.

Our custom wrapper around render via createClientRender has proofed invalueable in iterating on more involved testing approaches (StrictMode, test profiling, performance via Emotion cache busting etc). This was made possible by a testing approach that ultimately didn't care about the exact React tree but only the rendered component under test.

However, for matching up server-side rendering for hydration we do need to know the exact shape of the React component tree. Hydration always had the requirement of having to match component trees exactly but we could loosen this up a bit. We can no longer looses this up in React 18 with useId (see reactwg/react-18#111 (comment) and its thread for more context).

The "render same DOM on server and client" still holds. Turns out we just encountered a React bug: facebook/react#22681
But it's safer to render the exact same thing regardless. And the new API makes it impossible to accidentally cause hydration mismatches in a test (unless the component under test causes hydration mismatches).

Specifically, when testing hydration before we could use something like

const render = createClientRender()

test('hydration', () => {
  container.innerHTML = ReactDOMServer.renderToString(<App />);
  render(<App />, { container, hydrate: true } );
})

This pattern is replaced by

const { renderToString } = createRenderer()

test('hydration', () => {
  const { hydrate } = renderToString(<App />);
  hydrate();
})

The benefit of the new API is that we can continue to add arbitrary wrappers in render (or renderTostring) while hydrate returns the exact same shape as render.

By hiding the usage of renderToString we're also free to swap implementations of the used server renderer (renderToString should be replaced by the streaming renderer).

Regarding naming:

createClientRender was always supposed to be createRender. But when we introduce createClientRender we already had createRender based on Enyme's render. Since we removed createRender based on Enzyme in the meantime, we're free to use the originally intended name "createRender(er)".

Follow-up:

  • Remove createServerRender in favor of createRenderer().renderToString

@mui-pr-bot
Copy link

mui-pr-bot commented Nov 2, 2021

No bundle size changes

Generated by 🚫 dangerJS against d4d5184

Should replace createClientRender.
We can no longer use ReactDOMServer.renderToString() since server and client markup have to match.
Since test/utils adds some wrapper, we can no longer pass the same element to renderToString and clientRender.
@eps1lon eps1lon changed the title [test] Introduce new createRenderer API [test] Replace createClientRender with new createRenderer API Nov 2, 2021
@eps1lon eps1lon marked this pull request as ready for review November 2, 2021 19:55
@eps1lon eps1lon requested a review from a team November 3, 2021 08:26
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 Awesome improvement!

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation and giving us the history :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants