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

feat!: necessary api changes for React 19 compatibility #10

Merged
merged 38 commits into from
Dec 4, 2024

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented Nov 26, 2024

This release has to break some old APIs to stay compatible with React 19.

render is now async and should be awaited

This is the core change - due to the impementation of sibling prerendering on the React side, rendering has become more async and tests that interacted with render in a synchronous fashion would end up with not-resolving suspense boundaries.

Please adjust your tests accordingly:

  const {takeRender, render} = createRenderStream({ /* ... */ })
-  const utils = render(<Counter />)
+  const utils = await render(<Counter />)

enforcing of IS_REACT_ACT_ENVIRONMENT == false

In combination with issues we have have seen in the past, we have deduced that the testing approach of this library only really works in a "real-world" scenario, not in an act environment.

As a result, we will now throw an error if you try to use it in an environment where IS_REACT_ACT_ENVIRONMENT is truthy.

We are shipping a new tool, disableActEnvironment to prepare your environment for the duration of a test in a safe manner.

This helper can either be used with explicit resource management using the using keyword:

test('my test', () => {
  using _disabledAct = disableActEnvironment()

  // your test code here

  // as soon as this scope is left, the environment will be cleaned up
})

of by manually calling cleanup:

test('my test', () => {
  const {cleanup} = disableActEnvironment()

  try {
    // your test code here
  } finally {
    cleanup()
  }
})

This function does not only adjust your IS_REACT_ACT_ENVIRONMENT value, but it will also temporarily adjust the @testing-library/dom configuration in a way so that e.g. calls to userEvent will not automatically be wrapped in act.

Of course you can also use this tool on a per-file basis instead of a per-test basis, but keep in mind that doing so will break any React Testing Library tests that might be in the same file.

render is now it's own implementation

Previously, we used the render function of React Testing Library, but with the new restrictions this is no longer possible and we are shipping our own implementation.

As a result, some less-common options are not supported in the new implementation.
If you have a need for these, please let us know!

  • hydrate was removed
  • legacyRoot was removed. If you are using React 17, it will automatically switch to ReactDOM.render, otherwise we will use createRoot

Caution

React 17 does not look for IS_REACT_ACT_ENVIRONMENT to determine if it is running in an act-environment, but rather typeof jest !== "undefined".
If you have to test React 17, we recommend to patch it with a patch-package patch

renderToRenderStream was removed

As you now have to await the render call, renderToRenderStream had no real value anymore.

Where previously, the second line of

const renderStream = renderToRenderStream(<Component />, combinedOptions)
// this was optional in the past
const utils = await renderStream.renderResultPromise

could be omitted and could save you some code, now that second line would become required.

This now was not any shorter than calling createRenderStream and renderStream.render instead, and as both of these APIs now did the same thing in a different fashion, this would lead to confusion to no more benefit, so the API was removed.

Copy link

pkg-pr-new bot commented Nov 26, 2024

yarn@berry undefined https://pkg.pr.new/testing-library/react-render-stream-testing-library/@testing-library/react-render-stream@10

commit: 4d2345b

@phryneas phryneas changed the title [WIP] don't use act breaking: required api adjustments for React 19 compatibility Dec 3, 2024
@phryneas phryneas changed the title breaking: required api adjustments for React 19 compatibility BREAKING CHANGE: required api adjustments for React 19 compatibility Dec 3, 2024
@phryneas phryneas changed the title BREAKING CHANGE: required api adjustments for React 19 compatibility BREAKING CHANGE: required api changes for React 19 compatibility Dec 3, 2024
@phryneas phryneas changed the title BREAKING CHANGE: required api changes for React 19 compatibility BREAKING CHANGE: necessary api changes for React 19 compatibility Dec 3, 2024
@phryneas phryneas changed the title BREAKING CHANGE: necessary api changes for React 19 compatibility breaking!: necessary api changes for React 19 compatibility Dec 3, 2024
@phryneas phryneas changed the title breaking!: necessary api changes for React 19 compatibility feat!: necessary api changes for React 19 compatibility Dec 3, 2024
@phryneas phryneas marked this pull request as ready for review December 3, 2024 13:48
@phryneas phryneas requested a review from jerelmiller December 3, 2024 13:48
@phryneas phryneas changed the base branch from main to alpha December 3, 2024 13:54
Copy link
Collaborator

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

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

Had a few suggestions, but nothing major. Great job investigating all of this!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
*
* @default true
*/
adjustTestingLibConfig?: boolean
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
adjustTestingLibConfig?: boolean
patchTestingLibConfig?: boolean

Perhaps just me, but I prefer "patch" instead of "adjust" since its a bit more technical term.

Copy link
Member Author

Choose a reason for hiding this comment

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

patch would imply to me "we do something creepy with internals that was not intended", while this really just sets two config options.
I want to have this sound a little more harmless :)

Comment on lines +80 to +94
{
const previous = typedGlobal.IS_REACT_ACT_ENVIRONMENT
cleanupFns.push(() => {
Object.defineProperty(typedGlobal, 'IS_REACT_ACT_ENVIRONMENT', {
value: previous,
writable: true,
configurable: true,
})
})
Object.defineProperty(
typedGlobal,
'IS_REACT_ACT_ENVIRONMENT',
getNewPropertyDescriptor(false, preventModification),
)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
{
const previous = typedGlobal.IS_REACT_ACT_ENVIRONMENT
cleanupFns.push(() => {
Object.defineProperty(typedGlobal, 'IS_REACT_ACT_ENVIRONMENT', {
value: previous,
writable: true,
configurable: true,
})
})
Object.defineProperty(
typedGlobal,
'IS_REACT_ACT_ENVIRONMENT',
getNewPropertyDescriptor(false, preventModification),
)
}
const previous = typedGlobal.IS_REACT_ACT_ENVIRONMENT
cleanupFns.push(() => {
Object.defineProperty(typedGlobal, 'IS_REACT_ACT_ENVIRONMENT', {
value: previous,
writable: true,
configurable: true,
})
})
Object.defineProperty(
typedGlobal,
'IS_REACT_ACT_ENVIRONMENT',
getNewPropertyDescriptor(false, preventModification),
)

Are these brackets needed?

Copy link
Member Author

@phryneas phryneas Dec 4, 2024

Choose a reason for hiding this comment

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

Not 100% needed, but they are intentional here to restrict the scope of previous and group the core functionality together - that's why I lead it with a comment. It also makes it collapsible in the editor as a bonus, which is something I actively use.

// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
Symbol.dispose ?? Symbol.for('nodejs.dispose')

export interface DisableActEnvironmentOptions {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should these options be mentioned in the README?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would honestly not - there's a bunch of other, probably less used, options I don't mention in the README.

I see the README as the tool to quickly pick it up and JSDoc as API documentation here.

src/renderStream/createRenderStream.tsx Outdated Show resolved Hide resolved
@phryneas phryneas merged commit a3b3275 into alpha Dec 4, 2024
8 checks passed
@phryneas phryneas deleted the pr/noActRender branch December 4, 2024 09:13
phryneas added a commit that referenced this pull request Dec 4, 2024
BREAKING CHANGE: This release has to break some old APIs to stay compatible with React 19.

\#\# `render` is now `async` and should be `await`ed

This is the core change - due to the impementation of sibling prerendering on the React side, rendering has become more `async` and tests that interacted with `render` in a synchronous fashion would end up with not-resolving suspense boundaries.

Please adjust your tests accordingly:

```diff
  const {takeRender, render} = createRenderStream({ /* ... */ })
-  const utils = render(<Counter />)
+  const utils = await render(<Counter />)
```

\#\# enforcing of `IS_REACT_ACT_ENVIRONMENT == false`

In combination with [issues we have have seen in the past](facebook/react#29855), we have deduced that the testing approach of this library only really works in a "real-world" scenario, not in an `act` environment.

As a result, we will now throw an error if you try to use it in an environment where `IS_REACT_ACT_ENVIRONMENT` is truthy.

We are shipping a new tool, `disableActEnvironment` to prepare your environment for the duration of a test in a safe manner.

This helper can either be used with explicit resource management using the `using` keyword:

```ts
test('my test', () => {
  using _disabledAct = disableActEnvironment()

  // your test code here

  // as soon as this scope is left, the environment will be cleaned up
})
```

of by manually calling `cleanup`:

```ts
test('my test', () => {
  const {cleanup} = disableActEnvironment()

  try {
    // your test code here
  } finally {
    cleanup()
  }
})
```

This function does not only adjust your `IS_REACT_ACT_ENVIRONMENT` value, but it will also temporarily adjust the `@testing-library/dom` configuration in a way so that e.g. calls to `userEvent` will not automatically be wrapped in `act`.

Of course you can also use this tool on a per-file basis instead of a per-test basis, but keep in mind that doing so will break any React Testing Library tests that might be in the same file.

\#\# `render` is now it's own implementation

Previously, we used the `render` function of React Testing Library, but with the new restrictions this is no longer possible and we are shipping our own implementation.

As a result, some less-common options are not supported in the new implementation.
If you have a need for these, please let us know!

* `hydrate` was removed
* `legacyRoot` was removed. If you are using React 17, it will automatically switch to `ReactDOM.render`, otherwise we will use `createRoot`

> [!CAUTION]
> React 17 does not look for `IS_REACT_ACT_ENVIRONMENT` to determine if it is running in an `act`-environment, but rather `typeof jest !== "undefined"`.
> If you have to test React 17, we recommend to patch it with a [`patch-package` patch](https://github.com/apollographql/apollo-client/blob/8a4738a8ad7284d247513671628a4ac5917e104c/patches/react-dom-17+17.0.2.patch)

\#\#  `renderToRenderStream` was removed

As you now have to `await` the `render` call, `renderToRenderStream` had no real value anymore.

Where previously, the second line of
```js
const renderStream = renderToRenderStream(<Component />, combinedOptions)
// this was optional in the past
const utils = await renderStream.renderResultPromise
```
could be omitted and could save you some code, now that second line would become required.

This now was not any shorter than calling `createRenderStream` and `renderStream.render` instead, and as both of these APIs now did the same thing in a different fashion, this would lead to confusion to no more benefit, so the API was removed.
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