Fix #3648: observableRequiresReaction/computedRequiresReaction shouldn't warn with enableStaticRendering(true)
#3649
Conversation
🦋 Changeset detectedLatest commit: 2a6190b The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Hm seems it's gonna be tricker in relation to |
|
Why |
| expect(container).toHaveTextContent("0") | ||
| unmount() | ||
|
|
||
| expect(consoleWarnMock).toMatchSnapshot() |
| */ | ||
| allowStateReads = true | ||
| allowStateReads = false |
There was a problem hiding this comment.
This might be a breaking change? By default we allow state reads outside reactive contexts?
There was a problem hiding this comment.
No. These flags, allowStateReads and allowStateChanges, says whether you're in a context, where reads/changes are allowed - or perhaps better - where they are supposed to take place. They're basically inDerivation/inAction flags - they're switched on/off when entering/leaving these, regardless of whether it should warn or not. We use these flags instead of directly checking for action/derivation, because the mapping isn't always exactly 1:1. But really the point is to demark these places, not to directly control the warning - the warning is always only raised in combination with requiresReaction/enforceAction. They should always start as false, because there is no running action/derivation by default.
There was a problem hiding this comment.
Check, that adds up. I haven't been working on this for too long 😅.
There was a problem hiding this comment.
Turns out it's more complicated. I think it was originally designed as described, but it was maybe misunderstood and changed with the introduction of autoAction.
The thing is that autoAction is supposed to warn when you modify state inside derivation, regardless of enforceAction configuration. So autoAction assumes that if allowStateChanges is false, there will be a warning when setting state, no other conditions needed.
In order for this to work the configure/resetGlobalState was updated to synchronize allowStateChanges with enforeAction configuration.
But the actual check for the warning doesn't assume this behavior:
mobx/packages/mobx/src/core/derivation.ts
Lines 140 to 143 in 13a222e
Notice there is a bug - the check doesn't respect
enforceActions: "never" (enforceActions === false).This bug is actually a reason why the test for
autoAction warning is passing - there is a test above that calls mobx.configure({ enforceActions: "never" }).If you fix this bug, the
autoAction warning (Side effects like changing state are not allowed) can never occur.
|
Needs rebase |
|
@urugator I'm still waiting for this. Can you rebase and merge? |
|
Will try to take a look over the weekend. It will require some adaptation to the reworked observers. |
|
Update: I've spent a whole day on this, I've been dealing with some unanticipated issues. It might be possible to resolve your issue without bothering about these, not sure atm. If/when I find some more time I will try to focus on that. |
| @@ -295,27 +296,6 @@ test("issue 12", () => { | |||
| expect(events).toEqual(["table", "row: coffee", "row: tea", "table", "row: soup"]) | |||
| }) | |||
|
|
|||
| test("changing state in render should fail", () => { | |||
There was a problem hiding this comment.
This test is wrong on multiple levels:
I don't recall changing state in render should ever throw, with a slight exception of autoAction, which doesn't work ATM as mentioned elsewhere.
It doesn't expect(...).toThrow(), it just checks for the error (that is never thrown).
If it's about enforceAction, it should expect console.warn instead of an error.
It pollutes console with Warning: Cannot update a component while rendering a different component (this is a bit surprising given that only one component is involved, but not a concern atm).
|
Update: Should be ready for re-review + merge and we may tackle the other issues elsewhere. |
Fixes #3648
Changed
computedRequiresReactionto respectglobalState.allowStateReads.