Fix #3671: observer + StrictMode

[urugator]

No description provided.

[changeset-bot]

🦋 Changeset detected

Latest commit: 36eeb2f

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-lite	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]

Changes in render counts in deprecated tests are caused by React 18.0.0 -> 18.2.0. They were failing with pevious (pre 4.*) impl as well. See #3601 (comment)

[mweststrate]

Put up a draft that avoids making props / state observable, so that it plays better with Reacts internals. This diff shows a) alternative patterns and b) a way to detect the otherwise now broken usage of props / state inside reactions / computed. 94d78da

[urugator]

props/state/context are no longer observable.
Accessing props/state/context in different derivation than current render throws.
Fixed #3691

TODO:
Documentation.

Tests demonstrating workaround.

Shouldn't we throw instead?

if (componentClass[isMobXReactObserverSymbol]) {
        const displayName = getDisplayName(componentClass)
        console.warn(
            `The provided component class (${displayName})
                has already been declared as an observer component.`
        )
} else {
       componentClass[isMobXReactObserverSymbol] = true
}

Extending observer component doesn't seem to be supported, should we add an explicit check?

[mweststrate]

Shouldn't we throw instead?

Sounds good to me!

Extending observer component doesn't seem to be supported, should we add an explicit check?

Good idea, nails down that semantics at least.

What can I do to help further push this change :) ?

[urugator]

What can I do to help further push this change :) ?

Documenting the non-reactive props change would be a big help :)

[urugator]

Added tests demonstrating observable props/state/context workaround.

Applying observer multiple times now throws.

Explicit check for extending observer component turned out to be impossible without an access to observer base class constructor. Added note to documentation instead.

What would be nice to have, but I personally don't plan to do at the moment (help welcome):

• Test or sample app that integrates multiple functional / class components + strict mode and combines multiple techniques/approaches.
• Nightly releases, so these "bigger" changes could be tested on larger existing codebases before releasing major for production use.

What should be done next:

• Document non-reactive props/state/context change - motivation + workarounds. When done, replace TODO here https://github.com/mobxjs/mobx/pull/3673/files#diff-43fae443fda65b95af0b56e07002a6ba8cefd6ca87a8c62933bc2ffd6a742a9cR255
• Do a proper release - the previous failed release jobs did not published release notes, so these need to be somehow revived.
• Update compatibility matrix
  https://github.com/mobxjs/mobx/blob/main/packages/mobx-react/README.md#compatibility-matrix
  https://github.com/mobxjs/mobx/blob/main/packages/mobx-react-lite/README.md#compatibility-table-major-versions

[mweststrate]

• Document non-reactive props/state/context change - motivation + workarounds. When done, replace TODO here https://github.com/mobxjs/mobx/pull/3673/files#diff-43fae443fda65b95af0b56e07002a6ba8cefd6ca87a8c62933bc2ffd6a742a9cR255
• Update compatibility matrix
  https://github.com/mobxjs/mobx/blob/main/packages/mobx-react/README.md#compatibility-matrix
  https://github.com/mobxjs/mobx/blob/main/packages/mobx-react-lite/README.md#compatibility-table-major-versions
• Test or sample app that integrates multiple functional / class components + strict mode and combines multiple techniques/approaches.
• Nightly releases, so these "bigger" changes could be tested on larger existing codebases before releasing major for production use.
• Fix changelog not detecting the major bump on mobx-react
• Do a proper release - the previous failed release jobs did not published release notes, so these need to be somehow revived.
• Fix unit tests

See #3718 as I don't think I can push to origin urugator.

Will continue on remaining items coming days.

[mweststrate]

@urugator some items are still open, but how'd you feel about releasing the current state? Since I don't entirely trust the current state of releases / changelog, I'm tempted to trigger a release, and then verify is everything ended up well and roll forward if anything went south.

[urugator]

go for it

[mweststrate]

Merged, via #3718. Thanks for all the hard work here once again @urugator! (Feel free to invoice).

[mweststrate]

@urugator just noticed no mobx-react-lite release was triggered. Should this have emitted a major there as well?

[mweststrate]

Ah no, I think it worked.