feat: Use concurrent React when available #925

eps1lon · 2021-06-08T17:46:29Z

TODO:

how to tell the coverage report that we've reached full coverage across the whole build matrix i.e. with different versions of React?
By telling codecov that different React versions act as feature flags

Revisiting #507

BREAKING CHANGE: Concurrent features will be used by default if supported by the installed version of react-dom. To opt-out use `render(element, { legacyRoot: true }).

What:

Closes #509

Why:

Concurrent features will be available in React 18.

Opt-out (breaking change) vs opt-in (feature release):
createRoot is considered to be the default in React 18 so we should follow.

How:

Add a concurrent option to render. Note that there are still some unsolved problems with testing concurrent features like reactwg/react-18#21 (comment) and reactwg/react-18#23 (comment). So testing concurrent features will change. We will only support the latest pre-release of React 18. So any release of @testing-library/react may break your testing setup if don't have the latest alpha/beta/release candidate installed.

Checklist:

Documentation added to the
docs site
Tests
Typescript definitions updated
Ready to be merged

codesandbox-ci · 2021-06-08T17:47:45Z

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.

Latest deployment of this branch, based on commit 2ac2377:

Sandbox	Source
React	Configuration
react-testing-library-examples	Configuration

codecov · 2021-06-08T19:23:11Z

Codecov Report

Merging #925 (f570722) into alpha (05c7421) will decrease coverage by 5.08%.
The diff coverage is 85.71%.

❗ Current head f570722 differs from pull request most recent head 2ac2377. Consider uploading reports for the commit 2ac2377 to get more accurate results

@@             Coverage Diff             @@
##             alpha     #925      +/-   ##
===========================================
- Coverage   100.00%   94.91%   -5.09%     
===========================================
  Files            4        4              
  Lines          140      177      +37     
  Branches        28       34       +6     
===========================================
+ Hits           140      168      +28     
- Misses           0        8       +8     
- Partials         0        1       +1

Flag	Coverage Δ
latest	`94.91% <85.71%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/pure.js	`89.41% <85.71%> (-10.59%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05c7421...2ac2377. Read the comment docs.

kentcdodds · 2021-06-08T22:05:07Z

how to tell the coverage report that we've reached full coverage across the whole build matrix i.e. with different versions of React?

I believe that codecov can combine coverage reports, but I don't think there's a reasonable way to make Jest happy about our coverage number in a single environment 😬

kentcdodds

Thanks for your work on this and for your work in the working group! I have a few questions.

kentcdodds · 2021-06-08T22:11:56Z

src/pure.js

+  }
+
+  let root
+  // eslint-disable-next-line no-negated-condition -- we want to map the evolution of this over time. The root is created first. Only later is it re-used so we don't want to read the case that happens later first.


I think the rule is only complaining about the fact that this code could be written like this:

if (mountedContainers.has(container)) { // the forEach here } else { // create the root impl here. }

This was my interpretation as well. But I wanted to map the code to the lifetime of the root/container tuple as well:

gets created

gets reused

ESLint is telling me to put the "gets reused" branch first which is not what happens in time. So if you're reading the code top-down you don't know where the root/container tuple is coming from.

I should rewrite the comment since I tried to explain the reason there.

Basically, I don't think no-negated-condition does anything useful. I could just as well write

const needsRoot = !mountedContainers.has(container) if (needsRoot) { // create the root impl here. } else { // the forEach here }

which means I have to name one more thing but then I don't have to use the noisy eslint-disable with confusing comments.

kentcdodds · 2021-06-08T22:14:06Z

src/pure.js

+function waitFor(callback, options) {
+  return waitForDTL(() => {
+    let result
+    act(() => {
+      result = callback()
+    })
+    return result
+  }, options)
+}
+
+function waitForElementToBeRemoved(callback, options) {
+  return waitForElementToBeRemovedDTL(() => {
+    let result
+    act(() => {
+      result = callback()
+    })
+    return result
+  }, options)


I take it that these are added here because the removal of asyncWrapper?

Yeah, updates aren't flushed until we exit the outermost act. asyncWrapper was wrapping the whole waitFor call so the screen didn't ever get updated.

I'm not sure if we shouldn't actually change the dom-testing-library implementation so that we end up with

act(() => { jest.advanceTimersByTime(interval) })

or fork it entirely.

And we'd still have the issue of real timers: reactwg/react-18#23 (comment)

kentcdodds · 2021-06-08T22:14:29Z

src/pure.js

-  asyncWrapper: async cb => {
-    let result
-    await asyncAct(async () => {
-      result = await cb()
-    })
-    return result
-  },


So to be clear, there's no longer the concept of an async act anymore?

As far as I understand from reactwg/react-18#21 (comment), async act still exists in React 18, but sync act won't use microtasks so you won't need to flush them with await.

So to be clear, there's no longer the concept of an async act anymore?

Nothing changed in that regard.

I needed to remove to wrapper because it was used to wrap all the polling in waitFor:

await ReactDOMTestUtils.act(async () => { await new Promise((resolve) => { setInterval(async () => { if ( // That's the condition we wait for document.querySelector("main").getAttribute("aria-busy") !== "true" ) { resolve(); } }, 50); }); });

and this meant that the screen isn't actually updated during the polling i.e. it'll always reject and then the screen gets updated. This may be fine since we could just check one more time after the timeout but then interval becomes useless and the function runtime is always equal to timeout.

tests/setup-env.js

nickmccurdy · 2021-06-08T22:52:41Z

Since React 18 is publishing new releases nightly to both the next and experimental tags, should we only use one of those two tags for now (to save CI resources as they're both the same builds of React 18 alpha) and run daily scheduled builds (so we can catch regressions in React alphas even if there isn't a commit to Testing Library)? I can try this in a separate PR if you'd like.

nickmccurdy · 2021-06-08T23:00:50Z

src/pure.js

-  asyncWrapper: async cb => {
-    let result
-    await asyncAct(async () => {
-      result = await cb()
-    })
-    return result
-  },


As far as I understand from reactwg/react-18#21 (comment), async act still exists in React 18, but sync act won't use microtasks so you won't need to flush them with await.

nickmccurdy · 2021-06-08T23:04:37Z

.github/workflows/validate.yml

@@ -16,6 +16,7 @@ jobs:
    # ignore all-contributors PRs
    if: ${{ !contains(github.head_ref, 'all-contributors') }}
    strategy:
+      fail-fast: false


Maybe this should only be enabled in next/experimental React tag builds to keep latest tag builds fast.

I would land this change regardless of this PR. Personally, I'd like to know if a job only fails in one job. Otherwise how do I know what's causing the issue?

src/pure.js

nickmccurdy · 2021-06-08T23:29:57Z

src/pure.js

+  {
+    container,
+    baseElement = container,
+    legacyRoot = typeof ReactDOM.createRoot !== 'function',


createRoot is considered to be the default in React 18 so we should follow.

Defaulting to using createRoot could cause unexpected test failures for users that upgrade from React 17 to 18, but are still using the legacy render function for some reason (such as wanting to keep dependencies up to date without using concurrent mode or A/B testing with and without concurrent mode). I think it may be safer to leave it off, and have it be an intentional upgrade (like replacing render with createRoot in React itself). Then in the future if legacy render is removed, we could remove the option (replacing it with the current conditional check). That being said, I'm not sure how common these differences in behavior would be, and it could help to test it out with existing React 17 apps first.

Defaulting to using createRoot could cause unexpected test failures for users that upgrade from React 17 to 18

I think this risk will be eventually very low. In the end this PR will land in a major release so breakage should be expected. I just don't think it's healthy if React considers one API the default while we consider another one the default. Ultimately you can always wrap our render with a different default value. In jest you could just mock it which is what I did originally: https://github.com/eps1lon/react-testing-library/blob/04e865c0009a0d709a76846c80cdda683ac4b0fc/src/__mocks__/pure.js

Small update: React now logs a deprecation warning in React 18 if you use ReactDOM.render or ReactDOM.hydrate. I think we should not trigger warnings by default so the "createRoot by default" approach is going in the right direciton in my opinion.

eps1lon · 2021-06-09T04:21:19Z

Since React 18 is publishing new releases nightly to both the next and experimental tags, should we only use one of those two tags for now (to save CI resources as they're both the same builds of React 18 alpha) and run daily scheduled builds (so we can catch regressions in React alphas even if there isn't a commit to Testing Library)?

As far as I know experimental and next release channels will stay so I wouldn't want to change something temporarily. Especially since I'm not aware of any CI ressource problems we have.

nickmccurdy · 2021-06-09T08:08:22Z

Since React 18 is publishing new releases nightly to both the next and experimental tags, should we only use one of those two tags for now (to save CI resources as they're both the same builds of React 18 alpha) and run daily scheduled builds (so we can catch regressions in React alphas even if there isn't a commit to Testing Library)?

As far as I know experimental and next release channels will stay so I wouldn't want to change something temporarily. Especially since I'm not aware of any CI ressource problems we have.

What do you think about adding scheduled events for daily builds (without changing the tags)?

kentcdodds · 2021-06-11T20:48:43Z

FYI, I'm going out of town for a week so don't hold this up on my account.

eps1lon · 2021-06-12T08:07:45Z

FYI, I'm going out of town for a week so don't hold this up on my account.

The reason I'm hesitant is that waitFor and findBy doesn't really work anymore with React 18. I'm hoping this gets answered soon but I'd rather release the current state of alpha first and defer this PR to another major release.

Otherwise we can't see directly which version is failing

These only exists as invariants.

…ng-library#929)" This reverts commit c1878a9.

eps1lon · 2021-06-23T06:39:52Z

No github the branch is there again so why can't I re-open this? Well thanks for loosing context. Continued in #937

eps1lon added enhancement New feature or request BREAKING CHANGE This change will require a major version bump labels Jun 8, 2021

eps1lon changed the base branch from main to alpha June 8, 2021 17:49

eps1lon force-pushed the feat/concurrent-render branch from 8910fa2 to c83e9c1 Compare June 8, 2021 18:05

eps1lon marked this pull request as ready for review June 8, 2021 19:32

eps1lon requested a review from kentcdodds June 8, 2021 19:34

kentcdodds reviewed Jun 8, 2021

View reviewed changes

nickmccurdy reviewed Jun 8, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

nickmccurdy mentioned this pull request Jun 11, 2021

Investigation: try React-Redux v7 / v8 with React 18 alpha reduxjs/react-redux#1732

Closed

flybayer mentioned this pull request Jun 11, 2021

[legacy-framework] (newapp) Upgrade to react 18 alpha blitz-js/blitz#2478

Closed

eps1lon added 10 commits June 13, 2021 18:14

React 18 compat

46a743e

concurrent: false -> legacyRoot: true

70d2b34

fix logic for default of used root implementation

0e069a9

Add links to reactwg discussions

d83e34a

fix waitFor* in concurrent Reat

bc9ef29

cleanup

08c7932

Let full matrix build

2676666

Otherwise we can't see directly which version is failing

Ignore coverage for now

a06995d

Mark scheduler mocking as TODO

7e7e0d0

Merge codecov reports across react versions

1b201e0

eps1lon added 2 commits June 13, 2021 18:15

Exclude internal bugs from coverage

a776ef6

These only exists as invariants.

Add failing tests for waitFor and findBy

f570722

eps1lon force-pushed the feat/concurrent-render branch from 0a52d49 to f570722 Compare June 13, 2021 16:15

eps1lon mentioned this pull request Jun 13, 2021

Support React 18 which uses ReactDOM createRoot instead of render #933

Closed

eps1lon added 2 commits June 16, 2021 10:18

{ hydrate: true } -> hydrateRoot

00d534e

Revert "test: Ignore React 18 legacy root deprecation warnings (testi…

2ac2377

…ng-library#929)" This reverts commit c1878a9.

eps1lon closed this Jun 23, 2021

eps1lon deleted the branch testing-library:alpha June 23, 2021 06:32

eps1lon mentioned this pull request Jun 23, 2021

feat: Use concurrent React when available #937

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Use concurrent React when available #925

feat: Use concurrent React when available #925

eps1lon commented Jun 8, 2021 •

edited

codesandbox-ci bot commented Jun 8, 2021 •

edited

codecov bot commented Jun 8, 2021 •

edited

kentcdodds commented Jun 8, 2021

kentcdodds left a comment

kentcdodds Jun 8, 2021

eps1lon Jun 9, 2021

kentcdodds Jun 8, 2021

eps1lon Jun 9, 2021

kentcdodds Jun 8, 2021

nickmccurdy Jun 8, 2021

eps1lon Jun 9, 2021

nickmccurdy commented Jun 8, 2021 •

edited

nickmccurdy Jun 8, 2021

nickmccurdy Jun 8, 2021 •

edited

eps1lon Jun 9, 2021

nickmccurdy Jun 8, 2021

eps1lon Jun 9, 2021

eps1lon Jun 12, 2021

This comment has been minimized.

eps1lon commented Jun 9, 2021

This comment has been minimized.

nickmccurdy commented Jun 9, 2021 •

edited

kentcdodds commented Jun 11, 2021

eps1lon commented Jun 12, 2021

eps1lon commented Jun 23, 2021

feat: Use concurrent React when available #925

feat: Use concurrent React when available #925

Conversation

eps1lon commented Jun 8, 2021 • edited

codesandbox-ci bot commented Jun 8, 2021 • edited

codecov bot commented Jun 8, 2021 • edited

Codecov Report

kentcdodds commented Jun 8, 2021

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickmccurdy commented Jun 8, 2021 • edited

Choose a reason for hiding this comment

nickmccurdy Jun 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

eps1lon commented Jun 9, 2021

This comment has been minimized.

nickmccurdy commented Jun 9, 2021 • edited

kentcdodds commented Jun 11, 2021

eps1lon commented Jun 12, 2021

eps1lon commented Jun 23, 2021

eps1lon commented Jun 8, 2021 •

edited

codesandbox-ci bot commented Jun 8, 2021 •

edited

codecov bot commented Jun 8, 2021 •

edited

nickmccurdy commented Jun 8, 2021 •

edited

nickmccurdy Jun 8, 2021 •

edited

nickmccurdy commented Jun 9, 2021 •

edited