Better React 18 support #3590

urugator · 2022-12-17T21:56:33Z

mobx

It now keeps track of a global state version, which updates with each mutation.

mobx-react-lite

It now uses useSyncExternalStore, which should get rid of tearing (you have to update mobx, otherwise it should behave as previously).
Replaced reaction tracking utils with UniversalFinalizationRegistry. It works the same way, but I found it hard to orient myself in the original impl, so I rewrote it completely, hopefully for the better. It's also easier to reuse for class components.
Improved displayName/name handling as discussed in #3438.

mobx-react (class component)

Reactions of uncommited components are now correctly disposed. (fixes #3492)
Reactions don't notify uncommited components, avoiding the warning. (fixes #3492)
Removed symbol "polyfill" and replaced with actual Symbols.
Removed this.render replacement detection + warning. this.render is no longer configurable/writable (possibly BC *).
Reaction is no longer exposed as component[$mobx] (possibly BC *)
Component instance is no longer exposed as component[$mobx]["reactComponent"] (possibly BC *)
Deprecated disposeOnUnmount, it's not compatible with remounting.
Refactored code.
Fixed tests.

(*) BC for non-idiomatic usage or when depending on low level (private?) API.

I will update changeset once the changes settles.

changeset-bot · 2022-12-17T21:56:36Z

🦋 Changeset detected

Latest commit: e55c51c

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

This PR includes changesets to release 3 packages

Name	Type
mobx	Minor
mobx-react-lite	Major
mobx-react	Major

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-12-18T12:30:28Z

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

@@ -35,23 +37,7 @@ exports[`Redeclaring an existing observer component as an observer should log a
 }
 `;

-exports[`SSR works #3448 1`] = `


Was not supposed to warn. Warning was caused by calling disableStaticRendering before unmount in the test. Therefore componentWillUnmount used logic for non-static rendering.

urugator · 2022-12-18T12:32:26Z

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

@@ -102,7 +102,7 @@ describe("inject based context", () => {
        expect(C.displayName).toBe("inject(ComponentC)")
    })

-    test.only("shouldn't change original displayName of component that uses forwardRef", () => {


Was left in here for quite some time by mistake.

urugator · 2022-12-18T13:02:25Z

packages/mobx-react/src/observerClass.ts

-    /**
-     * If props are shallowly modified, react will render anyway,
-     * so atom.reportChanged() should not result in yet another re-render
-     */
-    setHiddenProp(this, skipRenderKey, false)
-    /**
-     * forceUpdate will re-assign this.props. We don't want that to cause a loop,
-     * so detect these changes
-     */
-    setHiddenProp(this, isForcingUpdateKey, false)


Merged these together into isUpdating as they basically handle the same thing:
One is set by reaction to prevent reportChanged on props setter,
second is set by props setter to prevent reaction.
So both says "I am already updating, do not cause another update"

…tore

mweststrate

Looking good so far! I didn't review everything yet (still have to do the new observer implementations), but submitting this review already so that you can see the comments, as coming days I probably won't be able to review further with all Christmas activities :).

Merry Christmas and happy new year!

packages/mobx-react-lite/src/utils/UniversalFinalizationRegistry.ts

packages/mobx-react/src/disposeOnUnmount.ts

packages/mobx/src/core/atom.ts

mweststrate · 2022-12-24T10:27:13Z

packages/mobx-react-lite/package.json

-    "dependencies": {},
+    "dependencies": {
+        "use-sync-external-store": "^1.2.0"
+    },
    "peerDependencies": {
        "mobx": "^6.1.0",


peer dependency probably has to be bumped to make sure globalState.stateVersion is available?

The current impl should be BC, see
https://github.com/mobxjs/mobx/pull/3590/files#diff-6b93715e19eb9119588dbe2af609a132062e61484d0d3de41b9e32e2d4a4ad5cR13-R17
https://github.com/mobxjs/mobx/pull/3590/files#diff-6b93715e19eb9119588dbe2af609a132062e61484d0d3de41b9e32e2d4a4ad5cR31-R34
https://github.com/mobxjs/mobx/pull/3590/files#diff-6b93715e19eb9119588dbe2af609a132062e61484d0d3de41b9e32e2d4a4ad5cR76-R79

but bumping dep is a way too.

Oh that is pretty neat actually! I'd still bump the peerDependency (as the warnings are often ignored), and make this change itself a major version, since it might affect semantics and spreads risk a bit?

If this is gonna be a major should it still be focused on React 18 support with otherwise minimal changes, or should this be an opportunity to introduce larger changes? (removing deprecated APIs, hooks, inject, options, cleanups, etc ...)

I assume the former.

I think we can do the former; decorators are also around the corner, an I expect after that is a better moment to do a bigger clean up (e.g. legacy decorator support etc)

mweststrate · 2022-12-24T10:28:44Z

packages/mobx/__tests__/v5/base/observables.js

+
+test("state version updates correctly", () => {
+    // This test was designed around the idea of updating version only at the end of batch,
+    // which is NOT an implementation we've settled on, but the test is still valid.


Sounds correct to me though :)

I am not sure what you mean. Just to provide some background. I've been looking for a common place where I could update state version. endBatch (when nesting level gets back to zero) seemed like a good candidate, because it's actually called even for individual mutations, but I was worried about some edge cases, like autorun invoking itself. So I wrote this test and gave it a go. However I quickly realized endBatch is also often called without any actual mutation, therefore I threw the idea away and moved the version update to reportChanged, but I kept the test as is. I've left the comment to clarify why the test looks this way.

mweststrate · 2022-12-24T10:58:07Z

Every Symbol is unique, so at least requires an allocation and GC.

…

On Sat, 24 Dec 2022, 11:54 Eric Masiello, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/mobx/src/core/atom.ts <#3590 (comment)>: > @@ -66,6 +67,9 @@ export class Atom implements IAtom { public reportChanged() { startBatch() propagateChanged(this) + // We could update state version only at the end of batch, + // but we would still have to switch some global flag here to signal a change. + globalState.stateVersion = Symbol() What makes you say Symbol() is more expensive? — Reply to this email directly, view it on GitHub <#3590 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBH7IACA5LFB6ZLNHUTWO3I5HANCNFSM6AAAAAATCE5TG4> . You are receiving this because you commented.Message ID: ***@***.***>

…tore

packages/mobx-react-lite/package.json

urugator · 2023-03-12T21:23:54Z

packages/mobx-react/package.json

    },
    "peerDependencies": {
-        "mobx": "^6.1.0",
+        "mobx": "^6.9.0",


A guess - I don't know the actual version it's gonna be released with. Dunno what's the correct way to handle it.

I forgot how to do that properly as well :-P But I think this should work anyway

urugator · 2023-03-12T21:25:58Z

packages/mobx-react/package.json

+        "mobx": "^6.8.0",
+        "mobx-react-lite": "^3.4.3",


Should become ^6.9?.0 and ^4.0.0 at the time of release. Again not sure how to handle that.

urugator · 2023-03-12T21:26:29Z

packages/mobx-react/package.json

@@ -36,10 +36,10 @@
    },
    "homepage": "https://mobx.js.org",
    "dependencies": {
-        "mobx-react-lite": "^3.4.0"
+        "mobx-react-lite": "^3.4.3"


Should become ^4.0.0 at the time of release. Again not sure how to handle that.

I think you can already update it now, since in a local checkout it is symlinked anyway to the right disk location?

If that causes problems; I'm not sure anymore if this happened automatically or not when releasing a master, but otherwise we can immediately release it with a patch afterwards?

urugator · 2023-03-12T21:29:47Z

@mweststrate Please take a look at #3649 and let me know if I should merge it to master first and incorporate the change to this PR.

mweststrate · 2023-03-16T10:51:20Z

Sorry for the delay! I'll dive into it this weekend

mweststrate

Looking good! Imho it's okay to merge #3649 after this one.

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

mweststrate · 2023-03-19T14:52:13Z

packages/mobx-react/package.json

@@ -36,10 +36,10 @@
    },
    "homepage": "https://mobx.js.org",
    "dependencies": {
-        "mobx-react-lite": "^3.4.0"
+        "mobx-react-lite": "^3.4.3"


I think you can already update it now, since in a local checkout it is symlinked anyway to the right disk location?

If that causes problems; I'm not sure anymore if this happened automatically or not when releasing a master, but otherwise we can immediately release it with a patch afterwards?

mweststrate · 2023-03-19T14:52:40Z

packages/mobx-react/package.json

    },
    "peerDependencies": {
-        "mobx": "^6.1.0",
+        "mobx": "^6.9.0",


I forgot how to do that properly as well :-P But I think this should work anyway

mweststrate

AWESOME STUFF!

ericmasiello · 2023-03-21T19:40:11Z

I pulled this repo locally and configured it to use this branch.

I see mostly progress with 1 regression. In the output below, "mobx" is mobx 6.8.0 and mobx-react-lite 3.4.0. "mobx-beta" uses the latest from this branch.

The only regressions I see are:

Can interrupt render (time slicing)

  mobx
    With useTransition
      Level 1
        ✓ No tearing finally on update (4571 ms)
        ✓ No tearing finally on mount (5472 ms)
      Level 2
        ✕ No tearing temporarily on update (8641 ms)
        ✕ No tearing temporarily on mount (5460 ms)
      Level 3
        ✓ Can interrupt render (time slicing) (3586 ms)
        ✕ Can branch state (wip state) (2952 ms)
    With useDeferredValue
      Level 1
        ✓ No tearing finally on update (9622 ms)
        ✓ No tearing finally on mount (6486 ms)
      Level 2
        ✓ No tearing temporarily on update (14596 ms)
        ✕ No tearing temporarily on mount (5468 ms)
  mobx-beta
    With useTransition
      Level 1
        ✓ No tearing finally on update (7975 ms)
        ✓ No tearing finally on mount (4555 ms)
      Level 2
        ✓ No tearing temporarily on update (12924 ms)
        ✓ No tearing temporarily on mount (4514 ms)
      Level 3
        ✕ Can interrupt render (time slicing) (7911 ms)
        ✕ Can branch state (wip state) (6685 ms)
    With useDeferredValue
      Level 1
        ✓ No tearing finally on update (9647 ms)
        ✓ No tearing finally on mount (4535 ms)
      Level 2
        ✓ No tearing temporarily on update (14596 ms)
        ✓ No tearing temporarily on mount (4494 ms)

urugator · 2023-03-21T20:52:56Z

@ericmasiello Cool, thank you for looking into it. Eventually I would love to see mobx as part of that repo. The results are what I was expecting/hoping for. I am not entirely sure about it, but I think level 3 is basically non-supportable by any kind of global state managment, because to do time slicing, you need to be able to split the state into indepedent pieces, which is impossible without any extra knowledge about the state. Eg in redux you would need two stores, two dispatchers etc, who knows absolutely nothing about one another. In mobx you would have to be able to somehow group observables, each group would maintain own version, but probably also own dependency tree, something like isolateGlobalState maybe? For state branching you probably need immutability.
The "regression" is most likely false positive on current version - without version checking, react is absolutely unware of observables, there is nothing that would tell him: No you can't do that, because it would cause some inconsistency in the output.

…tore

ericmasiello · 2023-03-22T13:12:12Z

@ericmasiello Besides mentioned fixed issues for which we do have tests, you should see improvements if you run it against: https://github.com/dai-shi/will-this-react-global-state-work-in-concurrent-rendering see this comment #2526 (comment) Personally I did not try and I don't have any setup for this, so at this point there is no easily reproducible evidence and you're welcome to change that :)

~~FYI, I have an open PR to add the latest release of mobx/mobx-react-lite to this repo dai-shi/will-this-react-global-state-work-in-concurrent-rendering#75~~

Correction: it merged!

jzhan-canva · 2023-05-12T00:14:58Z

Hi @urugator
can you please elaborate why disposeOnUnmount is deprecated?
I tested it in react 18, it seems the function version is working fine if it's in didMount (so the reaction is recreated when remount)
Is it only the decorator version problematic in react 18? (I tested it too, it seems can't dispose the reaction properly)

Shall we only deprecate the decorator version?

urugator · 2023-05-12T09:00:11Z

@zhantx As you said it would have to be called from componentDidMount:

Substantially limits usability and the initial motivation for this utility is no longer fully realized.
We would have to provide some means guarding against misuse (probably doable only partially and complicates things further).
useEffect hook was designed to solve this disposer collocation problem. If the problem was not worth solving for class components by react team, it's probably also not worth solving by us.
I think the general consensus at this point is to use reactions/autoruns sparingly and ideally keep them outside components, which presumably makes this utility less needed. Also just it's existence encourages the opposite.
I think it's a niche utility that doesn't provide so much value. It doesn't hide much complexity, it's basically just a different syntax, imo not even drastically shorter.

EDIT: removed the last point as I realized it requires patching componenWillUnmount which I don't consider trivial.

upsuper · 2023-05-12T10:17:17Z

Just want to mention that disposeOnUnmount is very useful for us, and it's widely used in our codebase. I'd also note that this utility doesn't encourage, nor does lack of it discourage the pattern. There exists lots of old code that simply collects disposers in componentDidMount and invokes them in componentWillUnmount. disposeOnUnmount helps simplify the code and ensure that they are disposed properly.

mweststrate · 2023-05-12T11:30:05Z

Note that disposeOnUnmount doesn't do anything MobX specific IIRC (it was just a utility we needed back then as well), so I think you could move it easily outside to your own utility function?

…

On Fri, May 12, 2023 at 12:17 PM Xidorn Quan ***@***.***> wrote: Just want to mention that disposeOnUnmount is very useful for us, and it's widely used in our codebase. I'd also note that this utility doesn't encourage, nor does lack of it discourage the pattern. There exists lots of old code that simply collects disposers in componentDidMount and invokes them in componentWillUnmount. disposeOnUnmount helps simplify the code and ensure that they are disposed properly. — Reply to this email directly, view it on GitHub <#3590 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBCSFZ3DEXVVR7A2ELLXFYE3RANCNFSM6AAAAAATCE5TG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

urugator · 2023-05-12T11:37:59Z

@upsuper
The utility was introduced specifically for disposing Mobx's resources - that is reaction/autorun, it would never exist otherwise.
Is it useful for other cases? Probably. Should Mobx be concerned about these. No. Mobx isn't toolkit for solving generic react problems.
So either it's for disposing autoruns/reactions, then it automaticaly raises the question why we expose a tool which makes it eaiser to use these in components, while at the same time discouraging from doing so.
Or it's not for disposing autoruns/reactions, then it's not a mobx problem and it shouldn't be part of this package (except BC).

That being said, as long as you're willing to be responsible for maintaing this tool, making sure it will be compatible with future react version (so we aren't in a similar situation year later), making sure it won't generate issues, complicate internals and people will use it correctly, I personally don't mind.

We've introduced some of these features in the past to be a bit more convinient here and there, to keep everyone happy and now we are reaping the fruits - we're dealing with React incompatibilities, piling up workarounds on the top of workarounds and introducing breaking changes.

upsuper · 2023-05-12T13:27:28Z

I think it's fine that we maintain our own version of this utility in our codebase. We use it both for reaction/autorun and for other stuff following a similar pattern.

Mainly we want to understand the reason that it's deprecated, so that we know whether our use of it has any fundamental issue, in which case our own version of this utility would suffer the same problem as well.

It's good to know that it isn't that the utility really has anything incompatible with React.

urugator · 2023-05-12T13:40:14Z

A potential source of some trouble is the need to patch componenWillUnmount, which we already patch by https://github.com/mobxjs/mobx/blob/main/packages/mobx-react/src/utils/utils.ts#L125
The complexity involved is beyond my current knowledge.

urugator added 4 commits November 26, 2022 23:40

wip

223ce6b

wip

fbe7604

wip

6976dec

wip

08ab24d

urugator added 6 commits December 18, 2022 11:01

fix test

951b7e8

wip

abfaa7e

refactor

b5c26ca

refactor

dfb75fd

refactors

5006263

refactors

229fc05

urugator commented Dec 18, 2022

View reviewed changes

urugator added 5 commits December 18, 2022 17:16

refactor

5ca1cfe

use useSyncExternalStore shim

fa05836

typo

8b8d948

refactor

27560d4

Merge remote-tracking branch 'upstream/main' into use-external-sync-s…

09e471a

…tore

urugator mentioned this pull request Dec 18, 2022

Support concurrent features React / React 18 #2526

Closed

mweststrate reviewed Dec 24, 2022

View reviewed changes

urugator added 3 commits December 31, 2022 14:52

useObserver: handle re-render without reaction

dba0d23

disposeOnUnmount jsdoc decprecation msg

2316e04

disposeOnUnmount refactor

dd52381

urugator mentioned this pull request Dec 31, 2022

useObserver registry refactor #3598

Merged

Merge remote-tracking branch 'upstream/main' into use-external-sync-s…

df38953

…tore

ericmasiello reviewed Jan 15, 2023

View reviewed changes

packages/mobx-react-lite/package.json Outdated Show resolved Hide resolved

urugator mentioned this pull request Jan 28, 2023

Next release #3601

Merged

tonyraoul mentioned this pull request Feb 1, 2023

Fix ide typescript compatability in __tests__ dir #3620

Merged

urugator added 3 commits March 12, 2023 21:57

bump deps

4ebb124

deps?

1c7e16c

deps?

4b138b5

urugator commented Mar 12, 2023

View reviewed changes

urugator marked this pull request as ready for review March 12, 2023 21:30

urugator requested a review from mweststrate March 16, 2023 07:49

mweststrate approved these changes Mar 19, 2023

View reviewed changes

mweststrate approved these changes Mar 20, 2023

View reviewed changes

urugator added 6 commits March 22, 2023 13:08

add new lines to changeset

21a7590

remove test

91e6c1a

Merge remote-tracking branch 'upstream/main' into use-external-sync-s…

26fe732

…tore

improve comment

12ef4cc

make finalization regsitry test bit more robust

98cde8b

increase timeout

e55c51c

urugator merged commit 44a2cf4 into mobxjs:main Mar 25, 2023

github-actions bot mentioned this pull request Mar 25, 2023

Next release #3661

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better React 18 support #3590

Better React 18 support #3590

urugator commented Dec 17, 2022 •

edited

Loading

changeset-bot bot commented Dec 17, 2022 •

edited

Loading

urugator Dec 18, 2022

urugator Dec 18, 2022

urugator Dec 18, 2022

mweststrate left a comment

mweststrate Dec 24, 2022

urugator Dec 28, 2022 •

edited

Loading

mweststrate Jan 3, 2023

urugator Feb 5, 2023

mweststrate Mar 20, 2023

mweststrate Dec 24, 2022

urugator Dec 28, 2022

mweststrate commented Dec 24, 2022 via email

urugator Mar 12, 2023

mweststrate Mar 19, 2023

urugator Mar 12, 2023 •

edited

Loading

urugator Mar 12, 2023

mweststrate Mar 19, 2023

urugator commented Mar 12, 2023

mweststrate commented Mar 16, 2023

mweststrate left a comment

mweststrate Mar 19, 2023

mweststrate Mar 19, 2023

mweststrate left a comment

ericmasiello commented Mar 21, 2023 •

edited

Loading

urugator commented Mar 21, 2023 •

edited

Loading

ericmasiello commented Mar 22, 2023 •

edited

Loading

jzhan-canva commented May 12, 2023 •

edited

Loading

urugator commented May 12, 2023 •

edited

Loading

upsuper commented May 12, 2023

mweststrate commented May 12, 2023 via email

urugator commented May 12, 2023 •

edited

Loading

upsuper commented May 12, 2023

urugator commented May 12, 2023

Better React 18 support #3590

Better React 18 support #3590

Conversation

urugator commented Dec 17, 2022 • edited Loading

mobx

mobx-react-lite

mobx-react (class component)

changeset-bot bot commented Dec 17, 2022 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mweststrate left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

urugator Dec 28, 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

mweststrate commented Dec 24, 2022 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

urugator Mar 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

urugator commented Mar 12, 2023

mweststrate commented Mar 16, 2023

mweststrate left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mweststrate left a comment

Choose a reason for hiding this comment

ericmasiello commented Mar 21, 2023 • edited Loading

urugator commented Mar 21, 2023 • edited Loading

ericmasiello commented Mar 22, 2023 • edited Loading

jzhan-canva commented May 12, 2023 • edited Loading

urugator commented May 12, 2023 • edited Loading

upsuper commented May 12, 2023

mweststrate commented May 12, 2023 via email

urugator commented May 12, 2023 • edited Loading

upsuper commented May 12, 2023

urugator commented May 12, 2023

urugator commented Dec 17, 2022 •

edited

Loading

changeset-bot bot commented Dec 17, 2022 •

edited

Loading

urugator Dec 28, 2022 •

edited

Loading

urugator Mar 12, 2023 •

edited

Loading

ericmasiello commented Mar 21, 2023 •

edited

Loading

urugator commented Mar 21, 2023 •

edited

Loading

ericmasiello commented Mar 22, 2023 •

edited

Loading

jzhan-canva commented May 12, 2023 •

edited

Loading

urugator commented May 12, 2023 •

edited

Loading

urugator commented May 12, 2023 •

edited

Loading