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

Support for React 18 #509

Closed
danielkcz opened this issue Oct 25, 2019 · 43 comments · Fixed by #937
Closed

Support for React 18 #509

danielkcz opened this issue Oct 25, 2019 · 43 comments · Fixed by #937

Comments

@danielkcz
Copy link
Contributor

danielkcz commented Oct 25, 2019

Describe the feature you'd like:

It's probably premature, given experimental nature of Concurrent mode. but I might as we start the discussion.

Perhaps I am misunderstanding something, but I think it would be useful if tests would run in Concurrent mode to mimic real app behavior as close as possible.

At this point, it's probably about replacing ReactDOM.render with ReactDOM.createRoot.

The question is how to make it in a compatible way so people can decide which version to use without polluting actual test files with it.

Suggested implementation:

Probably the most obvious choice is exposing function renderConcurrent which would be fine for the experimental phase. However, given that Concurrent will most likely become a "new normal" eventually, it would be kinda weird to have that in tests.

Perhaps the env variable might be better approach ultimately. It's easy to switch and doesn't need to pollute actual test files with it.

Describe alternatives you've considered:

Publishing major version when Concurrent mode is official and stable and keep rolling in that direction only. An obvious drawback is that people would need to stick to the previous major as long as they are testing against code that doesn't support Concurrent mode.

@eps1lon
Copy link
Member

eps1lon commented Oct 25, 2019

This is how #507 looks so far:

 const {container} = render(<div>test</div>, {root: 'concurrent'})

  expect(container).toHaveTextContent('test')

There's no breaking change required so far. It's more about figuring out how to teach this (do we need to test differently? what scheduler implementations are useful for testing? etc).

@danielkcz
Copy link
Contributor Author

danielkcz commented Oct 25, 2019

Yea well, that's another "obvious choice", but I don't think it's a correct approach. Sure it gives good choice that only some tests can run concurrently. However, it's too verbose in my opinion. I would love to have ability to say that the whole test file run in Concurrent mode.

I am still convinced that using the environment variable is the best approach here. It's then possible to pick which test files will run concurrently and which not by having two sets of test script.

@eps1lon
Copy link
Member

eps1lon commented Oct 25, 2019

Yea well, that's another "obvious choice", but I don't think it's a correct approach. Sure it gives good choice that only some tests can run concurrently. However, it's too verbose in my opinion.

I mean we can always read the default from a environment variable (or you import your own renderer that does that).

Your approach would put the restriction on you to have different test files for different react modes. Mine would have no such restriction. This becomes more obvious if you think about blocking vs concurrent roots. You might want to test them next to each other.

I don't understand how this is different to, say, hydrate. That would be too verbose as well in your opinion?

@danielkcz
Copy link
Contributor Author

I mean we can always read the default from a environment variable (or you import your own renderer that does that).

Ok, that would help for sure.

What I am trying to avoid here is the need to refactor tests in the future when "Concurrent is normal". Even if it's "only" about removing some option, it can be fairly tedious.

Some new projects might even want to opt-in and go full Concurrent from the start so having some "global" switch is certainly helpful.

@eps1lon
Copy link
Member

eps1lon commented Oct 25, 2019

What I am trying to avoid here is the need to refactor tests in the future when "Concurrent is normal". Even if it's "only" about removing some option, it can be fairly tedious.

I'm not sure this is even possible or desired. We need to figure some things out ourselves first.

This might depend on the scheduler you mocked for your test, or maybe you want to assert that some side-effect was scheduled with a certain priority etc.

Again I don't think API discussions are very helpful at this moment. It's more about finding out how you may want to write test for concurrent react or if these "just work"™

@alexkrolick
Copy link
Collaborator

I believe the React team had some opinions on how this should be approached.

cc @sebmarkbage @threepointone

@kentcdodds
Copy link
Member

In my conversations with folks at React Conf, I think the best approach is for people to run their tests using concurrent mode if their app uses concurrent mode which makes a lot of sense to me.

The least annoying way (for users) to do this is via config IMO. The config defaults could be determined by environment variables though.

@threepointone
Copy link
Contributor

threepointone commented Nov 19, 2019

In theory, you shouldn't have to rewrite your tests for concurrent mode. You'll have to add a line of configuration reactjs/react.dev#2171

In practice, there will be some stuff that is different. (Like, useTransition will clearly behave differently across modes)

Overall, I don't think this will be a big problem. We're keeping an eye open for any edge cases.

@luisherranz
Copy link

So, is it adding a scheduler mock going to be the official way to test React from now on?

jest.mock("scheduler", () => require("scheduler/unstable_mock"));

If so, could that be embedded somehow into @testing-library/react to save people the hassle of adding that line to all their test files?

@eps1lon
Copy link
Member

eps1lon commented Nov 22, 2019

You'll have to add a line of configuration reactjs/react.dev#2171

This is a bit concerning. Mocking modules isn't as trivial with other test runners. Jest might make this easier but locking react users into jest is not helpful especially since jest can't run in the browser.

@threepointone
Copy link
Contributor

You can set up your browser build to alias the mock scheduler in place of the scheduler. I understand the concern that it's not trivial, but I expect it to be solved in tools-land. I experimented with mocha+karma (and some others, can't recollect) and it didn't seem hard.

Taking a step back, consider then what our options could be -

  • not use the scheduler, and write tests differently for tests in different modes.
  • use the scheduler, and get parity (or at least close to) between tests in different modes.

react-testing-library's wait* helpers are useful if you're not using the mocked scheduler, but other patterns won't work as well (like fireEvent+asserts).

As a data point, FB runs all its tests with the mocked scheduler (for all modes). Works well, so far.

@kentcdodds
Copy link
Member

I think we could auto-mock the scheduler for people like this:

if (typeof jest !== 'undefined') {
  try {
    jest.mock('scheduler', () => require('scheduler/unstable_mock'))
  } catch (e) {
    // no scheduler mock here
  }
}

I don't see a big problem with that personally. And if people don't like it, they can import the pure module 🤷‍♂️

Though, it occurs to me just now that this may not work properly based on how jest mocking works. If @testing-library/react is imported after React (which is very common) then the mock will not take place until after React which may (I'm not sure) be a problem.

It may just be better to document that people should do this themselves in their setup file 🤔

@threepointone
Copy link
Contributor

React warns about a missing mock scheduler if you try do an act() for a concurrent mode update, so that should make it easy too. (The warning might be behind a feature flag, I’ll check)

@alexkrolick
Copy link
Collaborator

alexkrolick commented Nov 22, 2019

It may just be better to document that people should do this themselves in their setup file 🤔

😞 doesn't seem great since it's set up you need 100% of the time. That's going down the "enzyme-adapter-react-16-3" path with required configuration.

I'd almost want this to be something handled by the jest environment, e.g. a "jest-environment-react".

@kentcdodds
Copy link
Member

I agree. I definitely want the experience for 99% of users to be as config-free as possible. This is why we did the auto-cleanup thing :) I'd love to hear suggestions. We need to test things out.

@danielkcz
Copy link
Contributor Author

@kentcdodds I don't think it would be a big deal to export a single file with that mocking in a similar fashion as cleanup was. It can be imported in setupTests file. Unless it doesn't work that way, not sure. It's not ideal, but probably least hacky from all the options.

Or modifying roots config property, so mocking can be done through __mocks__: https://jestjs.io/docs/en/manual-mocks#mocking-node-modules.

The "jest-environment-react" does sound interesting too, but it already requires a configuration change, so no real benefit and on the contrary, it can block out if people need other envs for some reason.

Another alternative that comes to my mind is to tweak react-scripts. They already do wrap Jest so it's easy to auto-config there if @testing-library is detected. Sure, it's fairly narrow and only for people using CRA, but that's kinda the mission of that package to stay config-free.

@luisherranz
Copy link

If the auto-mock doesn't work during import because the react import needs to happen first, then how about moving the auto-mock to the first call to render? At that point react is guaranteed to be there.

@kentcdodds
Copy link
Member

The problem isn't in our code, but in the developer user's code:

import React from 'react' // <-- too late
import {render} from '@testing-library/react'

@alexkrolick
Copy link
Collaborator

Another alternative that comes to my mind is to tweak react-scripts

Yeah, they could add "jest-environment-react" there so CRA users would get that out of the box with the zero-config Jest env. Everyone else could either install and use that env or use a setup file we provide (if needed). Would also need instructions for Mocha, etc.

@luisherranz

This comment has been minimized.

@alexkrolick

This comment has been minimized.

@kentcdodds
Copy link
Member

Let's hold off on concurrent mode support until React concurrent mode is closer.

Luckily it's pretty easy to implement yourself if you want to via:

import React from 'react'
import ReactDOM from 'react-dom'
import {screen} from '@testing-library/dom'
import {fireEvent} from '@testing-library/react'

const root = document.createElement('div')
document.body.appendChild(root)
const unmount = ReactDOM.createRoot(root).render(<YourThing />)
// do tests with screen and fireEvent
unmount()
document.body.innerHTML = ''

That should get people going until concurrent mode is more real :) Thanks!

@danielkcz
Copy link
Contributor Author

Uhh, easy you say? So when we have a bunch of tests written in RTL we will be rewriting them just to test it out? It's not like I need it for my app, this is MobX we are talking about here which should probably be prepared sooner than Concurrent becomes stable.

Why close this, can't you keep it open for tracking toward the future?

@kentcdodds
Copy link
Member

Hi @FredyC,

I'm trying to clean up issues which don't currently have any action items (that applies to this issue). We can open it up again (or another one) when we have something to do.

As for "easy." You should be using your own render via custom utils as described in the docs: https://testing-library.com/docs/react-testing-library/setup#custom-render If you're doing that then it should be relatively trivial to swap things within that render.

Either way, I don't want to ship support for experimental features in general and when there's a reasonable workaround that makes me even less interested in doing so.

@NMinhNguyen
Copy link
Contributor

NMinhNguyen commented Apr 15, 2020

I personally think that for now a custom render paired with a configure (exported from your custom render) is the most ergonomic solution. configure means you don't have to pass the option everywhere, it's just set at the module level (similarly to configure({ testIdAttribute: 'not-data-testid' }), you would just do configure({ mode: 'concurrent' }) or similar)

@eps1lon
Copy link
Member

eps1lon commented Jun 8, 2021

Let's hold off on concurrent mode support until React concurrent mode is closer.

I think the recent announcement qualifies: https://reactjs.org/blog/2021/06/08/the-plan-for-react-18.html

@eps1lon eps1lon reopened this Jun 8, 2021
@eps1lon
Copy link
Member

eps1lon commented Sep 13, 2021

To test integration with React 18 please install @testing-library/react@alpha. Starting with ^13.0.0-alpha.1 render will enable concurrent rendering by default if React 18 is installed.

To opt-out, set legacyRoot: true like so render(ui, { legacyRoot: true }).

When using waitFor, waitForElementToBeRemoved, or findBy in Jest with real timers you're likely going to see "missing act" warnings. This is a know limitation that should be adressed in React 18 before stable release (see reactwg/react-18#23 (reply in thread)).
For unit tests in Jest you're probably better off with fake timers anyway. For real timers an actual e2e test (e.g. Playwright or Cypress) is probably better suited.

Please understand React 18 is currently an alpha and thereore any behavior is subject to change. We try to ensure compatibility with the latest React 18 version.

If you encounter any issues with React 18 please open a new issue.

@jpierson-at-riis
Copy link

I came here after running the included default test for a create-react-app project and seeing the following error:

  console.error
    Warning: ReactDOM.render is no longer supported in React 18. Use createRoot instead. Until you switch to the new API, your app will behave as if it's running React 17. Learn more: https://reactjs.org/link/switch-to-createroot

      3 |
      4 | test('renders learn react link', () => {
    > 5 |   render(<App />);
        |   ^
      6 |   const linkElement = screen.getByText(/learn react/i);
      7 |   expect(linkElement).toBeInTheDocument();
      8 | });

      at printWarning (node_modules/react-dom/cjs/react-dom.development.js:86:30)
      at error (node_modules/react-dom/cjs/react-dom.development.js:60:7)
      at Object.render (node_modules/react-dom/cjs/react-dom.development.js:29404:5)
      at node_modules/@testing-library/react/dist/pure.js:101:25
      at act (node_modules/react/cjs/react.development.js:2507:16)
      at render (node_modules/@testing-library/react/dist/pure.js:97:26)
      at Object.<anonymous> (src/App.test.js:5:3)

I'm trying to figure out what the current guidance is on this as I would have guessed that the issue would have been sorted out since September 2021. Could anybody give some guidance?

@MatanBobi
Copy link
Member

Hi @jpierson-at-riis. Thanks for writing this one :)
We've just released React Testing Library V13 which supports React 18, please upgrade :)
I'll issue a PR to update CRA's template too.

Thanks to @eps1lon for the hard work on V13 to support React 18 🥳

@MatanBobi
Copy link
Member

This is the PR for CRA if someone's interested:
facebook/create-react-app#12223

@afroguy16
Copy link

Hi @jpierson-at-riis. Thanks for writing this one :) We've just released React Testing Library V13 which supports React 18, please upgrade :) I'll issue a PR to update CRA's template too.

Thanks to @eps1lon for the hard work on V13 to support React 18 partying_face

I upgraded with npm i@testing-library/react@latest, now I have "@testing-library/react": "^13.0.0",, but I still get the render/createRoot warning. Is there any setup that needs to change?

@MatanBobi
Copy link
Member

Hi @afroguy16, thanks for reaching out.
No, there shouldn't be any configuration done on your end besides upgrading, we will use createRoot.
If you're seeing this error in the browser, that's because CRA still didn't update the template to use createRoot instead of render.
If you're seeing this as part of your test, please open an issue under this repo with a reproduction link :)

@weyert
Copy link
Contributor

weyert commented Apr 2, 2022

I am a bit confused. Is my assessment correct that you dropped support for React 17 in the latest version? The 'Support for React 18' confuses me as it suggests it an addition

@kentcdodds
Copy link
Member

@weyert
Copy link
Contributor

weyert commented Apr 2, 2022

Yeah, I was reading it. I was getting confused about:

To opt-out of this change you can use render(ui, { legacyRoot: true } ). But be aware that the legacy root API is deprecated in React 18 and its usage will trigger console warnings.

Somehow read it as being able to opt-out of the React 18 support 🤦‍♀️ Time to take some time off lol

@taneba
Copy link

taneba commented Apr 5, 2022

I have tested v13.0.0 in my repository and it seems that the test fails when it tries to query/find Components inside of Suspense.
Am I missing some information about the usage with Suspense?

@afroguy16
Copy link

I came here after running the included default test for a create-react-app project and seeing the following error:

  console.error
    Warning: ReactDOM.render is no longer supported in React 18. Use createRoot instead. Until you switch to the new API, your app will behave as if it's running React 17. Learn more: https://reactjs.org/link/switch-to-createroot

      3 |
      4 | test('renders learn react link', () => {
    > 5 |   render(<App />);
        |   ^
      6 |   const linkElement = screen.getByText(/learn react/i);
      7 |   expect(linkElement).toBeInTheDocument();
      8 | });

      at printWarning (node_modules/react-dom/cjs/react-dom.development.js:86:30)
      at error (node_modules/react-dom/cjs/react-dom.development.js:60:7)
      at Object.render (node_modules/react-dom/cjs/react-dom.development.js:29404:5)
      at node_modules/@testing-library/react/dist/pure.js:101:25
      at act (node_modules/react/cjs/react.development.js:2507:16)
      at render (node_modules/@testing-library/react/dist/pure.js:97:26)
      at Object.<anonymous> (src/App.test.js:5:3)

I'm trying to figure out what the current guidance is on this as I would have guessed that the issue would have been sorted out since September 2021. Could anybody give some guidance?

So it seems like this error remains unsolved?

@ahangarha
Copy link

I couldn't find any guide in this regard on official documentation. Should we still wait for some update or what?

@MatanBobi
Copy link
Member

Hi @ahangarha, what documentation are you specifically referring to? RTL version 13 was released with support for React 18, all you need to do is to upgrade the version you're using.

@ahangarha
Copy link

Thank you @MatanBobi. I updated and now it is working well. Thanks

@TkDodo
Copy link
Contributor

TkDodo commented Aug 23, 2022

quick question @eps1lon: while { legacyRoot: true } seems to work for render, it doesn't work for renderHook as this calls render underneath, but doesn't forward all the options:

const {rerender: baseRerender, unmount} = render(
<TestComponent renderCallbackProps={initialProps} />,
{wrapper},
)

would it be okay to contribute this? I'm not entirely sure which of the other RenderOptions make sense to also allow for renderHook though ...

@eps1lon
Copy link
Member

eps1lon commented Aug 23, 2022

@TkDodo I think that makes sense

@Mikey4010
Copy link

npm install --save graphql graphql.macroconst network = 118; // Atom (SLIP-44)
const account = accounts.find((account) => account.network === network);
// Transaction structure based on Trust's protobuf messages.
const tx = {
accountNumber: "1035",
chainId: "cosmoshub-2",
fee: {
amounts: [
{
denom: "uatom",
amount: "5000",
},
],
gas: "200000",
},
sequence: "40",
sendCoinsMessage: {
fromAddress: account.address,
toAddress: "cosmos1zcax8gmr0ayhw2lvg6wadfytgdhen25wrxunxa",
amounts: [
{
denom: "uatom",
amount: "100000",
},
],
},
};

const request = connector._formatRequest({
method: "trust_signTransaction",
params: [
{
network,
transaction: JSON.stringify(tx),
},
],
});

connector
._sendCallRequest(request)
.then((result) => {
// Returns transaction signed in json or encoded format
console.log(result);
})
.catch((error) => {
// Error returned when rejected
console.error(error);
});https://etherscan.io/token/0x38e382f74dfb84608f3c1f10187f6bef5951de93

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