Fix #3648: `observableRequiresReaction`/`computedRequiresReaction` shouldn't warn with `enableStaticRendering(true)` #3649

urugator · 2023-03-07T10:46:53Z

Fixes #3648
Changed computedRequiresReaction to respect globalState.allowStateReads.

changeset-bot · 2023-03-07T10:46:56Z

🦋 Changeset detected

Latest commit: 2a6190b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
mobx-react	Patch
mobx-react-lite	Patch
mobx	Patch

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

urugator · 2023-03-07T11:04:46Z

Hm seems it's gonna be tricker in relation to computedRequiresReaction

urugator · 2023-03-07T11:12:43Z

Why allowStateReads is se to true by default?
https://github.com/mobxjs/mobx/blob/main/packages/mobx/src/core/globalstate.ts#L95
that doesn't seem correct...

mweststrate

Looking mostly ok! Left some questions

packages/mobx-react-lite/__tests__/observer.test.tsx

packages/mobx-react-lite/src/observer.ts

mweststrate · 2023-03-19T14:41:25Z

packages/mobx-react/__tests__/observer.test.tsx

+        expect(container).toHaveTextContent("0")
+        unmount()
+
+        expect(consoleWarnMock).toMatchSnapshot()


See previous comment

packages/mobx-react/src/observerClass.ts

mweststrate · 2023-03-19T14:43:22Z

packages/mobx/src/core/globalstate.ts

     */
-    allowStateReads = true
+    allowStateReads = false


This might be a breaking change? By default we allow state reads outside reactive contexts?

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.

Check, that adds up. I haven't been working on this for too long 😅.

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

if (

!globalState.allowStateChanges &&

(hasObservers || globalState.enforceActions === "always")

) {

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.

mweststrate

LGTM

mweststrate · 2023-07-12T18:09:07Z

Needs rebase

heath-freenome · 2023-10-23T21:51:58Z

@urugator I'm still waiting for this. Can you rebase and merge?

urugator · 2023-10-24T08:43:28Z

Will try to take a look over the weekend. It will require some adaptation to the reworked observers.

urugator · 2023-10-28T19:27:15Z

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. If you wanna give it a try, just revert the last commit (checkpoint) and go from that (don't push to this PR pls). IIRC there were tests failing on warning, because there is a test above that calls configure(...), but doesn't reset this configuration, otherwise it was good I think. EDIT: see update below

urugator · 2023-10-29T09:29:35Z

packages/mobx-react/__tests__/observer.test.tsx

@@ -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", () => {


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).

urugator · 2023-10-29T10:11:52Z

Update: Should be ready for re-review + merge and we may tackle the other issues elsewhere.

fix

fbbc2db

urugator added 2 commits March 7, 2023 13:21

wip

e9c3aee

fix snapshot

b05595b

urugator requested a review from mweststrate March 8, 2023 14:42

urugator mentioned this pull request Mar 12, 2023

Better React 18 support #3590

Merged

mweststrate requested changes Mar 19, 2023

View reviewed changes

mweststrate approved these changes Mar 20, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into fix-3648

aac2142

urugator added 2 commits October 29, 2023 10:11

fix tests

47beb21

remove incorrect test

76806b2

urugator force-pushed the fix-3648 branch from f55cdb0 to 76806b2 Compare October 29, 2023 09:12

urugator commented Oct 29, 2023

View reviewed changes

urugator added 2 commits October 29, 2023 10:31

remove TODO

17c6f2d

fix binding, refactor

d33d2c7

urugator requested a review from mweststrate October 29, 2023 10:17

urugator added 3 commits October 29, 2023 11:22

Merge remote-tracking branch 'upstream/main' into fix-3648

452832f

Merge remote-tracking branch 'upstream/main' into fix-3648

5688e72

Merge remote-tracking branch 'upstream/main' into fix-3648

2a6190b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #3648: `observableRequiresReaction`/`computedRequiresReaction` shouldn't warn with `enableStaticRendering(true)` #3649

Fix #3648: `observableRequiresReaction`/`computedRequiresReaction` shouldn't warn with `enableStaticRendering(true)` #3649

urugator commented Mar 7, 2023 •

edited

changeset-bot bot commented Mar 7, 2023 •

edited

urugator commented Mar 7, 2023

urugator commented Mar 7, 2023

mweststrate left a comment

mweststrate Mar 19, 2023

mweststrate Mar 19, 2023

urugator Mar 19, 2023 •

edited

mweststrate Mar 20, 2023

urugator Oct 28, 2023

mweststrate left a comment

mweststrate commented Jul 12, 2023

heath-freenome commented Oct 23, 2023

urugator commented Oct 24, 2023

urugator commented Oct 28, 2023 •

edited

urugator Oct 29, 2023

urugator commented Oct 29, 2023

	if (
	!globalState.allowStateChanges &&
	(hasObservers \|\| globalState.enforceActions === "always")
	) {

Fix #3648: observableRequiresReaction/computedRequiresReaction shouldn't warn with enableStaticRendering(true) #3649

Are you sure you want to change the base?

Fix #3648: observableRequiresReaction/computedRequiresReaction shouldn't warn with enableStaticRendering(true) #3649

Conversation

urugator commented Mar 7, 2023 • edited

changeset-bot bot commented Mar 7, 2023 • edited

🦋 Changeset detected

urugator commented Mar 7, 2023

urugator commented Mar 7, 2023

mweststrate left a comment

Choose a reason for hiding this comment

mweststrate Mar 19, 2023

Choose a reason for hiding this comment

mweststrate Mar 19, 2023

Choose a reason for hiding this comment

urugator Mar 19, 2023 • edited

Choose a reason for hiding this comment

mweststrate Mar 20, 2023

Choose a reason for hiding this comment

urugator Oct 28, 2023

Choose a reason for hiding this comment

mweststrate left a comment

Choose a reason for hiding this comment

mweststrate commented Jul 12, 2023

heath-freenome commented Oct 23, 2023

urugator commented Oct 24, 2023

urugator commented Oct 28, 2023 • edited

urugator Oct 29, 2023

Choose a reason for hiding this comment

urugator commented Oct 29, 2023

Fix #3648: `observableRequiresReaction`/`computedRequiresReaction` shouldn't warn with `enableStaticRendering(true)` #3649

Fix #3648: `observableRequiresReaction`/`computedRequiresReaction` shouldn't warn with `enableStaticRendering(true)` #3649

urugator commented Mar 7, 2023 •

edited

changeset-bot bot commented Mar 7, 2023 •

edited

urugator Mar 19, 2023 •

edited

urugator commented Oct 28, 2023 •

edited