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

using the wrong renderer's act() should warn #15399

Conversation

threepointone
Copy link
Contributor

@threepointone threepointone commented Apr 12, 2019

via #15319

So tl;dr this PR -

  • uses a renderer specific object (specifically, the flushPassiveEffects function itself) as the value of ReactShouldWarnActingUpdates.current
  • checks this value on 'initial' render
  • checks this value when setting a state hook's value
  • adds a fixture folder for act()

This solves 2 specific problems -

  • using the 'wrong' act() shouldn't silence the warning:
    We do this by using a sigil unique to the renderer (as an elegant hack, we use flushPassiveEffects as the sigil, removing the need to make a new object). act() sets this object before the callback, and unsets it when it's over. Then in addition to warnIfNotCurrentlyActingUpdatesInDev(), we also check warnIfNotScopedWithMatchingAct() to confirm thatReactShouldWarnActingUpdates.current matches the expected sigil.

  • using the wrong act() logs a warning:
    Using the same above method, we can check whether you're using the right version of act() for your code.

Now, I first added this check only for state hook updates, but it didn't reliably catch the common failure case. Consider the following component -

function App() {
  let [state, setState] = useState(0);
  async function ticker() {
    await null;
    setState(x => x + 1);
  }
  useEffect(() => {
    ticker();
  }, [Math.min(state, 4)]);
  return state;
}

Let's write a test for it using the shiny new async act()

const el = document.createElement("div");
await ReactTestUtils.act(async () => {
  ReactDOM.render(React.createElement(App), el);
});
// all 5 ticks present and accounted for
console.log(el.innerHTML); // 5!

This is the golden path - use the correct act with the matching renderer, and you'll get expected behaviour. Here's a quick diagram of the timeline it goes through

image

Of note, because we can check every time after calling flushPassiveEffects(), we can guarantee that the act() 'scope' will stay open until the effects queue is drained. Good.

Now, let's use a mismatching act()-

await ReactTestUtils.act(async () => {
  ReactTestRenderer.create(React.createElement(App));
});

Now, let's say we'd added our sigil check only for updates, you'd think it would still trigger the warning. However, the timing of things has changed. There are 2 scenarios of how these will be sequenced out. The first, which is the 'good' version -

image

Because we can't use flushPassiveEffects() as expected (ie - it's just a no op for other renderer instances), we have to rely on the browser/jest environment to flush to the 'screen', and then the effects/updates fire. In this 'good' case, at least one set state call happens inside the act scope, so we can do the sigil check and warn that they aren't using the right act version.

However, the bad news is that this happens super rarely (in my rough estimation, only 1 in 20 'successes'). In reality, it usually happens outside the scope of the act() scope.

image

Here, you'll see that the effects fire after the act() scope has closed, so when we do the sigil check, we can only warn that the dev hasn't wrapped their code with act() (which will confuse them, since they think they already have)

So how do we fix this? Well, I noticed that most of these failures happen at the very start, when they initialise (TestRenderer.create(), ReactDOM.render(), etc). I believe that if we also add just the sigil identity check in the reconciler's .createContainer(), we should be able to warn for most cases asap that they're using the wrong version (and ofc, createContainer is synchronous and won't escape the act() scope).

A quirk with our codebase is we don't allow multiple renderers in the same test, so I made a fixture folder fixtures/act to test these scenarios (see index.test.js for details). I used react-dom / react-test-renderer for the tests but I should probably add react-art as well.

Questions -

  • In a previous iteration of this PR, I made a separate new object as the sigil, but then replaced it with using flushPassiveEffects as the sigil itself. ok/not ok?

Sunil Pai added 5 commits April 3, 2019 17:11
via facebook#15319
This solves 2 specific problems -
- using the 'wrong' act() doesn't silence the warning
- using the wrong act logs a warning

It does this by using an empty object on the reconciler as the identity of ReactShouldWarnActingUpdates.current. We also add this check when calling createContainer() to catch the common failure of this happening right in the beginning.
@sizebot
Copy link

sizebot commented Apr 12, 2019

ReactDOM: size: 0.0%, gzip: -0.0%

Details of bundled changes.

Comparing: 5731e52...26ce8b4

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.2% +0.3% 847.22 KB 848.83 KB 193.06 KB 193.63 KB UMD_DEV
react-dom.production.min.js 0.0% -0.0% 105.12 KB 105.12 KB 34.15 KB 34.15 KB UMD_PROD
react-dom.development.js +0.2% +0.3% 841.54 KB 843.15 KB 191.5 KB 192.06 KB NODE_DEV
react-dom.profiling.min.js 0.0% -0.0% 108.46 KB 108.46 KB 34.44 KB 34.44 KB NODE_PROFILING
ReactDOM-dev.js +0.2% +0.3% 867.04 KB 868.79 KB 193.08 KB 193.65 KB FB_WWW_DEV
ReactDOM-prod.js 0.0% 0.0% 353.02 KB 353.02 KB 65.32 KB 65.32 KB FB_WWW_PROD
ReactDOM-profiling.js 0.0% 0.0% 358 KB 358 KB 66.31 KB 66.31 KB FB_WWW_PROFILING
react-dom-unstable-fire.development.js +0.2% +0.3% 847.56 KB 849.17 KB 193.21 KB 193.78 KB UMD_DEV
react-dom-unstable-fire.production.min.js 0.0% -0.0% 105.13 KB 105.13 KB 34.15 KB 34.15 KB UMD_PROD
react-dom-unstable-fire.development.js +0.2% +0.3% 841.88 KB 843.49 KB 191.64 KB 192.2 KB NODE_DEV
react-dom-unstable-fire.profiling.min.js 0.0% -0.0% 108.47 KB 108.47 KB 34.45 KB 34.45 KB NODE_PROFILING
ReactFire-dev.js +0.2% +0.3% 866.25 KB 868 KB 193.01 KB 193.58 KB FB_WWW_DEV
ReactFire-prod.js 0.0% 0.0% 340.99 KB 340.99 KB 62.92 KB 62.92 KB FB_WWW_PROD
ReactFire-profiling.js 0.0% -0.0% 345.95 KB 345.95 KB 63.88 KB 63.88 KB FB_WWW_PROFILING
react-dom-test-utils.development.js +0.5% +0.6% 56.93 KB 57.22 KB 15.68 KB 15.77 KB UMD_DEV
react-dom-test-utils.production.min.js 0.0% -0.1% 10.76 KB 10.76 KB 3.95 KB 3.94 KB UMD_PROD
react-dom-test-utils.development.js +0.5% +0.6% 55.27 KB 55.56 KB 15.36 KB 15.46 KB NODE_DEV
react-dom-test-utils.production.min.js 0.0% -0.0% 10.51 KB 10.51 KB 3.88 KB 3.88 KB NODE_PROD
ReactTestUtils-dev.js +0.6% +0.6% 52.7 KB 53.02 KB 14.21 KB 14.3 KB FB_WWW_DEV
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 60.76 KB 60.76 KB 15.85 KB 15.85 KB UMD_DEV
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 60.43 KB 60.43 KB 15.72 KB 15.72 KB NODE_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.0% 10.43 KB 10.43 KB 3.57 KB 3.56 KB NODE_PROD
react-dom-server.browser.development.js 0.0% -0.0% 137.21 KB 137.21 KB 36.17 KB 36.17 KB UMD_DEV
react-dom-server.browser.production.min.js 0.0% -0.0% 19.2 KB 19.2 KB 7.23 KB 7.23 KB UMD_PROD
react-dom-server.browser.development.js 0.0% -0.0% 133.34 KB 133.34 KB 35.23 KB 35.23 KB NODE_DEV
ReactDOMServer-dev.js 0.0% -0.0% 135.54 KB 135.54 KB 34.8 KB 34.8 KB FB_WWW_DEV
ReactDOMServer-prod.js 0.0% -0.0% 47.98 KB 47.98 KB 11.04 KB 11.03 KB FB_WWW_PROD
react-dom-server.node.development.js 0.0% -0.0% 135.28 KB 135.28 KB 35.78 KB 35.78 KB NODE_DEV
react-dom-server.node.production.min.js 0.0% -0.0% 19.98 KB 19.98 KB 7.53 KB 7.53 KB NODE_PROD
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 3.81 KB 3.81 KB 1.54 KB 1.54 KB UMD_DEV
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 3.64 KB 3.64 KB 1.49 KB 1.49 KB NODE_DEV
react-dom-unstable-fizz.node.development.js 0.0% -0.1% 3.88 KB 3.88 KB 1.51 KB 1.51 KB NODE_DEV
react-dom-unstable-fizz.node.production.min.js 0.0% -0.1% 1.1 KB 1.1 KB 668 B 667 B NODE_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.3% +0.4% 583.12 KB 584.73 KB 127.61 KB 128.17 KB UMD_DEV
react-art.production.min.js 0.0% -0.0% 96.81 KB 96.81 KB 29.81 KB 29.81 KB UMD_PROD
react-art.development.js +0.3% +0.5% 514.03 KB 515.64 KB 110.11 KB 110.68 KB NODE_DEV
ReactART-dev.js +0.3% +0.5% 524.41 KB 526.16 KB 109.43 KB 110.01 KB FB_WWW_DEV
ReactART-prod.js 0.0% 0.0% 201.25 KB 201.25 KB 34.34 KB 34.34 KB FB_WWW_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.3% +0.4% 656.68 KB 658.44 KB 139.82 KB 140.41 KB RN_FB_DEV
ReactNativeRenderer-dev.js +0.3% +0.4% 656.59 KB 658.35 KB 139.79 KB 140.38 KB RN_OSS_DEV
ReactFabric-dev.js +0.3% +0.4% 645.43 KB 647.19 KB 137.08 KB 137.66 KB RN_FB_DEV
ReactFabric-prod.js 0.0% 0.0% 242.41 KB 242.41 KB 42.04 KB 42.04 KB RN_FB_PROD
ReactFabric-profiling.js 0.0% 0.0% 250.16 KB 250.16 KB 43.69 KB 43.69 KB RN_FB_PROFILING
ReactFabric-dev.js +0.3% +0.4% 645.34 KB 647.1 KB 137.05 KB 137.63 KB RN_OSS_DEV
ReactFabric-profiling.js 0.0% 0.0% 250.17 KB 250.17 KB 43.69 KB 43.69 KB RN_OSS_PROFILING

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.4% +0.6% 527.41 KB 529.31 KB 113 KB 113.64 KB UMD_DEV
react-test-renderer.production.min.js 0.0% -0.0% 63.08 KB 63.08 KB 19.47 KB 19.47 KB UMD_PROD
react-test-renderer.development.js +0.4% +0.6% 522.95 KB 524.85 KB 111.89 KB 112.53 KB NODE_DEV
react-test-renderer.production.min.js 0.0% -0.0% 62.76 KB 62.76 KB 19.23 KB 19.23 KB NODE_PROD
ReactTestRenderer-dev.js +0.4% +0.6% 535.39 KB 537.47 KB 111.93 KB 112.59 KB FB_WWW_DEV
react-test-renderer-shallow.development.js 0.0% -0.0% 41.95 KB 41.95 KB 10.83 KB 10.83 KB UMD_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.3% +0.5% 511.14 KB 512.75 KB 108.57 KB 109.13 KB NODE_DEV
react-reconciler.production.min.js 0.0% -0.0% 62.79 KB 62.79 KB 18.83 KB 18.83 KB NODE_PROD
react-reconciler-persistent.development.js +0.3% +0.5% 508.81 KB 510.42 KB 107.57 KB 108.14 KB NODE_DEV
react-reconciler-reflection.development.js 0.0% -0.0% 19.16 KB 19.16 KB 6.08 KB 6.08 KB NODE_DEV
react-reconciler-reflection.production.min.js 0.0% -0.2% 2.51 KB 2.51 KB 1.12 KB 1.11 KB NODE_PROD

Generated by 🚫 dangerJS

@threepointone
Copy link
Contributor Author

hmm @sizebot you have my attention

@acdlite
Copy link
Collaborator

acdlite commented Apr 12, 2019

@threepointone sizebot compares to the nearest common ancestor of master and your branch (e5c5935). Because you've merged from master several times since then, the size reduction includes stuff that has already landed. This is one of the reasons I prefer to rebase on top of upstream changes instead of merging them.

Sunil Pai added 3 commits April 12, 2019 23:39
made fixtures/act for act(). specifically, this lets us tests how act behaves when you use the wrong act() for the wrong renderer. also mvoes dom/../act-dom.html to this fixture.
@threepointone threepointone marked this pull request as ready for review April 14, 2019 14:30
@threepointone
Copy link
Contributor Author

threepointone commented Apr 14, 2019

What if I used flushPassiveEffects as the sigil value? 🤔 it would remove the need for the extra exports. EDIT - done.

acdlite and others added 6 commits April 15, 2019 14:08
* [sizebot] Fail gracefully if CI returns invalid response

Moves the `response.json()` call into the catch block.

* Stop tracking bundle sizes
…ons (facebook#15402)

The Pointer Events spec mentions that the value of `button` in a nativeEvent
can be anything between 0 and 5 for "down" events. We only care about those
with a value of 0.
via facebook#15319
This solves 2 specific problems -
- using the 'wrong' act() doesn't silence the warning
- using the wrong act logs a warning

It does this by using an empty object on the reconciler as the identity of ReactShouldWarnActingUpdates.current. We also add this check when calling createContainer() to catch the common failure of this happening right in the beginning.

make a proper fixture for act()

made fixtures/act for act(). specifically, this lets us tests how act behaves when you use the wrong act() for the wrong renderer. also mvoes dom/../act-dom.html to this fixture.

cleanup fixtures/dom/.gitignore

verify that it 'works' with art+dom

verify that it 'works' with art+test

augh prettier

tweak warning messages
…one/react into renderer-specific-act-warning
packages/react/src/ReactShouldWarnActingUpdates.js Outdated Show resolved Hide resolved
fixtures/act/package.json Outdated Show resolved Hide resolved
packages/react-test-renderer/src/ReactTestRendererAct.js Outdated Show resolved Hide resolved
@acdlite
Copy link
Collaborator

acdlite commented Apr 18, 2019

Because we can't use flushPassiveEffects() as expected (ie - it's just a no op for other renderer instances), we have to rely on the browser/jest environment to flush to the 'screen', and then the effects/updates fire.

Something to keep in mind is that passive effects are scheduled with Scheduler, and our recommendation will be to use the mock Scheduler build in tests. Which means effects that aren't wrapped by act will never be flushed; you can't rely on the "browser/jest environment" to eventually flush them in a subsequent event.

@acdlite
Copy link
Collaborator

acdlite commented Apr 18, 2019

^ I believe the reason you didn't encounter this problem in your tests is because the separate fixture test suite you created doesn't use the mock Scheduler build.

@acdlite acdlite mentioned this pull request Apr 27, 2019
9 tasks
@threepointone
Copy link
Contributor Author

@acdlite how would you feel about landing this before the flushAll() work anyway? It's useful enough that I want to make further PRs on top of it I think. I could make another branch off this but ehhh.

@threepointone
Copy link
Contributor Author

bump

@acdlite
Copy link
Collaborator

acdlite commented May 28, 2019

I can't review this until you resolve the conflicts

@threepointone
Copy link
Contributor Author

abandoning this for #15756

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

Successfully merging this pull request may close these issues.

8 participants