using the wrong renderer's act() should warn #15398
Closed
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closed this in favour of #15399
via #15319
This solves 2 specific problems -
using the 'wrong'
act()
shouldn't silence the warning:We do this by using a new empty object on the reconciler (called
ReactActingUpdatesSigil
, but I should rename that).act()
sets this object before the callback, and unsets it when it's over. ThenwarnIfNotCurrentActingUpdates()
not only checks whetherReactShouldWarnActingUpdates.current
is set, but also whether it matches the expected sigil.using the wrong
act()
logs a warningUsing 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 -
Let's write a test for it using the shiny new async
act()
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](https://user-images.githubusercontent.com/18808/56043174-a508a300-5d34-11e9-98c5-31f2f943de01.png)
Of note, because we can check everytime after calling
flushPassiveEffects()
, we can guarantee that we theact()
'scope' will stay open until the effects queue is drained. GOod.Now, let's use a mismatching
act()
-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 -
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.
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 theact()
scope).This PR does the above. I'm opening this PR as a draft just to make sure my approach is right. I haven't written jest tests yet, and was using my act-dom.html fixture for testing this. I'm adding jest tests asap. Maybe the problem I described above won't be as big problem in jest. Or maybe it will! It's an adventure to find out.
Also I don't like the variable name
ReactActingUpdatesSigil
. It's cool, but I'll save it for when I want to feel clever later.