Bugfix #3392 #3404

pixelkritzel · 2022-05-19T15:59:22Z

Bugfix #3392

Code change checklist

Added/updated unit tests
Updated /docs. For new functionality, at least API.md should be updated
Verified that there is no significant performance drop (yarn mobx test:performance)

changeset-bot · 2022-05-19T15:59:57Z

🦋 Changeset detected

Latest commit: 6875f53

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

This PR includes changesets to release 1 package

Name	Type
mobx-react	Minor

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 · 2022-05-19T16:52:50Z

run yarn changeset in root dir, choose mobx-react and minor (spacebar to check, enter to confirm)

urugator · 2022-05-19T17:11:26Z

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

@@ -908,3 +908,71 @@ test("Missing render should throw", () => {
    }
    expect(() => observer(Component)).toThrow()
 })
+
+test("this.context is not observable if ComponentName.contextType is not set", () => {
+    const store = observable({ counter: 0 })


You don't need any stores or observables here. All we have to test is:
Parent component has a state (eg number).
Update parent component state (eg increment the number).
Parent component passes this state (number) via context to child.
Child returns this.context from @computed.
Child returns @computed in render.
Prior fix the child's output won't correlate with parent's state.

It's better now?

urugator · 2022-05-20T15:28:22Z

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

+
+    console.log(compRef.current?.counterValue)
+
+    expect(compRef.current?.counterValue).toBe(1)


Have you tried to run the test without the fix? I think this would pass even before.

This passes in both cases. But this why I used the store before. Like here

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

Line 56 in 36d3d35

store = observable({

A test that passes before and aft doesn't sound like a good test :) I can't see the original test clearly anymore, would you mind adding that as well for now at least? I do think atm this PR is correct, but that it isn't the "correct" solution to the original problem, from scanning the original issues 😅

You can see the original test in this commit.

4088edd

To clarify; in the original problem your 'store' isn't a store at at all, it is an unstable object reference for something that should be stable in the mobx idioms, and the correcter way to solve it would be simple: <FormContext.Provider value={this}> and skip that whole computed thing. Computed values shouldn't normally be new objects that are observable / mutable; either they are a ref to an already existing object, or they are immutable value objects, just representing the derived value at a certain moment in time.

In your current setup you create many objects that might all live at the same moment in time, representing many different selections.

Edit: fixed snippet

Just use the computed value inside the render and it will fail. The computed acts as plain getter if it's not observed, therefore the problem doesn't occur.

Ah, for the first test, yeah that didn't seem to make sense indeed, that should pass even if context would be an observable.

mweststrate · 2022-05-20T16:46:54Z

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

+        }
+
+        render() {
+            return <div />


what you want to test here as well is <div>{this.counterValue}</div>, is getting rerendered (once) to make sure that the @observer as well picks up the change in the computed value, as createObservableProp sets some internal flags, for which I'm not 100% sure that is correct in the case of context (I suspect it is correct)

pixelkritzel · 2022-05-21T11:25:10Z

@urugator @mweststrate

New test is up.

It checks

the output of <Child> render function
how often Child.render runs.

The @computed on the parent is necessary to fail if Child.contextType is undefined. This is the same as original example.

pixelkritzel · 2022-05-21T11:29:00Z

And funny thing I discovered is this.context is always defined (even if contextType is not set) and an empty object which event overrides values set on the class.

https://codesandbox.io/s/heuristic-mayer-th9058?file=/src/App.tsx

mweststrate · 2022-05-21T11:34:57Z

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

+
+    let renderCounter = 0
+
+    @observer


Thanks this test looks a lot better! Still want to point out that this component doesn't have to be an observer at all if you'd put an observable object as context value, saving re-rendering the root of the tree and any unmemo-ed child thereunder, whenever you increase the counter. That is the way it is typically done, and I suspect also the reason why no one reported this earlier.

What difference does it make to the render passes whether the context value is in an observable or in a computed?

The component must react either way to pass the new value to the context.

In your setup, it is Parent that reacts to a change in counter, by creating a new object (at line 929) as part of the render process to compute this.contextValue (you basically make an unnecessary state copy here). If you'd pass a stable observable object (or even this) as Provider value, the counter itself isn't read at all by the Parent, but instead by the Child, so that the effect of changing counter would be much more local effect. So instead of re-rendering in principle the whole subtree of Parent, you'd only be re-rendering individual Child components.

The component must react either way to pass the new value to the context.

So, it is not necessary at all to pass a new value as context :) You don't have to allocate a new store if you can just keep using the same store and update it's internal state instead.

So basically cutting the Reacts render logic completely out and using parent only as Context Provider? This is kind of strict.

In real life I often have something like this:

@computed get contextValue(): DropdownContextType { return { toggleDropdown: this.toggleDropdown, closeDropdown: this.closeDropdown, open: this.open, type: this.props.type || this.computedType, }; }

and this.open is also accessed in render to determine if dropdown should be rendered open or not. So it doesn't matter here.

I don't see the benefit of constructing that object over and over again over just passing this (you strongly typed out the other members anyway). If you really want to strict down the object passed, you could also allocate it once and just make it a delegator object, so that you can keep using the same contextValue even when you're state changes over time:

contextValue = { toggleDropdown: this.toggleDropdown, closeDropdown: this.closeDropdown, get open() { return this.open }, get type() { return this.props.type || this.computedType } }

So basically cutting the Reacts render logic completely out and using parent only as Context Provider?

If there is no need for it yes, if there is need for it, fine as well. In you're cases shown so far there is no need (as in, there is no visual changes to your UI 'above' the Child components.

But this way you get into nasty this bugs.

https://codesandbox.io/s/admiring-stallman-hcnrpv?file=/src/Form.tsx

Thank you for your help and support! And for Mobx obviously! Luckily it came out early enough so that I didn't dipped to much in Redux.

pixelkritzel · 2022-05-22T16:40:17Z

@urugator can this PR merged?

urugator · 2022-05-23T07:25:59Z

Thank you

pixelkritzel added 2 commits May 19, 2022 17:53

Closes mobxjs#3392

4088edd

Merge branch 'mobxjs:main' into main

42db284

pixelkritzel mentioned this pull request May 19, 2022

mobx-react: computed properties aren't recomputed while accessing React context with static contextType #3392

Closed

pixelkritzel added 2 commits May 19, 2022 19:10

Adds changeset

5b6f31a

Merge branch 'main' of github.com:pixelkritzel/mobx

f86c65c

urugator reviewed May 19, 2022

View reviewed changes

Simplifies test

0ccf907

urugator reviewed May 20, 2022

View reviewed changes

mweststrate reviewed May 20, 2022

View reviewed changes

Adds working test

54c14fe

mweststrate reviewed May 21, 2022

View reviewed changes

Removes unused childRef

6bebe77

mweststrate approved these changes May 21, 2022

View reviewed changes

Merge branch 'main' into main

6875f53

urugator merged commit 1470b8e into mobxjs:main May 23, 2022

github-actions bot mentioned this pull request May 23, 2022

Next release #3413

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix #3392 #3404

Bugfix #3392 #3404

pixelkritzel commented May 19, 2022

changeset-bot bot commented May 19, 2022 •

edited

Loading

urugator commented May 19, 2022

urugator May 19, 2022

pixelkritzel May 20, 2022

urugator May 20, 2022

pixelkritzel May 20, 2022

mweststrate May 20, 2022

pixelkritzel May 20, 2022

mweststrate May 20, 2022

urugator May 20, 2022 •

edited

Loading

mweststrate May 20, 2022

mweststrate May 20, 2022

pixelkritzel commented May 21, 2022

pixelkritzel commented May 21, 2022

mweststrate May 21, 2022

pixelkritzel May 21, 2022

mweststrate May 21, 2022 •

edited

Loading

pixelkritzel May 21, 2022

pixelkritzel May 21, 2022

mweststrate May 21, 2022

pixelkritzel May 21, 2022

pixelkritzel May 21, 2022

pixelkritzel commented May 22, 2022

urugator commented May 23, 2022


		console.log(compRef.current?.counterValue)

		expect(compRef.current?.counterValue).toBe(1)

Bugfix #3392 #3404

Bugfix #3392 #3404

Conversation

pixelkritzel commented May 19, 2022

Code change checklist

changeset-bot bot commented May 19, 2022 • edited Loading

🦋 Changeset detected

urugator commented May 19, 2022

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

urugator May 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pixelkritzel commented May 21, 2022

pixelkritzel commented May 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mweststrate May 21, 2022 • edited Loading

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

pixelkritzel commented May 22, 2022

urugator commented May 23, 2022

changeset-bot bot commented May 19, 2022 •

edited

Loading

urugator May 20, 2022 •

edited

Loading

mweststrate May 21, 2022 •

edited

Loading