doc: Correct legacy version support for react #3828

[robhybrid]

I pulled down the old mobx-react repo for version 6, and updated the react dependencies to v17, but the tests fail. I don't think mobx-react v5 or v6 can support react v17.

[changeset-bot]

⚠️ No Changeset found

Latest commit: f319958

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[mweststrate]

I'm not sure that is correct, the tests fail is quite a broad stroke. Did everything fall apart or did just some tests fail? That might also relate to subtleties in test infra (e.g. jest version etc also play into these things)

[robhybrid]

I updated all the test infrastructure to to be compatible with React 17 before running the tests. 5 tests out 107 failed.

This PR is just an update to the documentation. The support table on npmjs.com shows that React v17 is supported by mobx-react v5 and v6, which isn't true. I've tried making them work together in the past, and I found it to be impossible. I made this change in response to this bug:

#3828

This bug is valid. I checked.
Either the docs need to be fixed, or add React 17 support needs to be added for mobx-react v4 and v5.

Here is a fork with the tests failing against React 17:
https://github.com/robhybrid/mobx-react

This is the test output:

PASS test/propTypes.test.ts
PASS test/symbol.test.tsx
FAIL test/issue806.test.tsx
  ● verify issue 806

    expect(jest.fn()).toHaveBeenCalledWith(...expected)

    Expected: "[mobx] Observable [email protected] being read outside a reactive context"
    Received: "[mobx] Observable '[email protected]' being read outside a reactive context."

    Number of calls: 1

      46 |         x.a.toString()
      47 |         expect(console.warn).toBeCalledTimes(1)
    > 48 |         expect(console.warn).toHaveBeenCalledWith(
         |                              ^
      49 |             "[mobx] Observable [email protected] being read outside a reactive context"
      50 |         )
      51 |     })

      at test/issue806.test.tsx:48:30
      at Object.withConsole (test/utils/withConsole.ts:20:11)
      at Object.<anonymous> (test/issue806.test.tsx:45:5)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)

PASS test/misc.test.tsx
PASS test/context.test.tsx
PASS test/transactions.test.tsx
PASS test/Provider.test.tsx
PASS test/disposeOnUnmount.test.tsx
PASS test/stateless.test.tsx
PASS test/hooks.test.tsx
  ● Console

    console.warn
      [mobx-react-lite] 'useAsObservableSource' is deprecated, please store the values directly in an observable, for example by using 'useLocalObservable', and sync future updates using 'useEffect' when needed. See the README for examples.

      13 | 
      14 |     const Child = ({ x }) => {
    > 15 |         const props = useAsObservableSource({ x })
         |                       ^
      16 |         const store = useLocalStore(() => ({
      17 |             get getPropX() {
      18 |                 return props.x

      at useDeprecated (node_modules/mobx-react-lite/src/utils/utils.ts:6:17)
      at Object.useAsObservableSource (node_modules/mobx-react-lite/src/useAsObservableSource.ts:6:5)
      at Child (test/hooks.test.tsx:15:23)
      at renderWithHooks (node_modules/react-dom/cjs/react-dom.development.js:14985:18)
      at mountIndeterminateComponent (node_modules/react-dom/cjs/react-dom.development.js:17811:13)
      at beginWork (node_modules/react-dom/cjs/react-dom.development.js:19049:16)
      at beginWork$1 (node_modules/react-dom/cjs/react-dom.development.js:23940:14)
      at performUnitOfWork (node_modules/react-dom/cjs/react-dom.development.js:22779:12)

    console.warn
      [mobx-react-lite] 'useLocalStore' is deprecated, use 'useLocalObservable' instead.

      14 |     const Child = ({ x }) => {
      15 |         const props = useAsObservableSource({ x })
    > 16 |         const store = useLocalStore(() => ({
         |                       ^
      17 |             get getPropX() {
      18 |                 return props.x
      19 |             }

      at useDeprecated (node_modules/mobx-react-lite/src/utils/utils.ts:6:17)
      at Object.useLocalStore (node_modules/mobx-react-lite/src/useLocalStore.ts:16:5)
      at Child (test/hooks.test.tsx:16:23)
      at renderWithHooks (node_modules/react-dom/cjs/react-dom.development.js:14985:18)
      at mountIndeterminateComponent (node_modules/react-dom/cjs/react-dom.development.js:17811:13)
      at beginWork (node_modules/react-dom/cjs/react-dom.development.js:19049:16)
      at beginWork$1 (node_modules/react-dom/cjs/react-dom.development.js:23940:14)
      at performUnitOfWork (node_modules/react-dom/cjs/react-dom.development.js:22779:12)

    console.error
      Warning: Cannot update a component (`Observer`) while rendering a different component (`Child`). To locate the bad setState() call inside `Child`, follow the stack trace as described in https://reactjs.org/link/setstate-in-render
          at Child (/Users/robertwilliams/vscode-projects/mobx-react/test/hooks.test.tsx:14:23)
          at Parent (/Users/robertwilliams/vscode-projects/mobx-react/test/hooks.test.tsx:28:41)

      13 | console.error = function(msg) {
      14 |     if (/react-wrap-tests-with-act/.test("" + msg)) throw new Error("missing act")
    > 15 |     return origError.apply(this, arguments as any)
         |                      ^
      16 | }
      17 | 

      at console.Object.<anonymous>.console.error (jest.setup.ts:15:22)
      at printWarning (node_modules/react-dom/cjs/react-dom.development.js:67:30)
      at error (node_modules/react-dom/cjs/react-dom.development.js:43:5)
      at warnAboutRenderPhaseUpdatesInDEV (node_modules/react-dom/cjs/react-dom.development.js:24002:15)
      at scheduleUpdateOnFiber (node_modules/react-dom/cjs/react-dom.development.js:21836:3)
      at setState (node_modules/react-dom/cjs/react-dom.development.js:16139:5)
      at forceUpdate (node_modules/mobx-react-lite/src/useObserver.ts:45:31)
      at Reaction.onInvalidate_ (node_modules/mobx-react-lite/src/useObserver.ts:73:17)

PASS test/issue21.test.tsx
FAIL test/inject.test.tsx
  ● inject based context › propTypes and defaultProps are forwarded

    expect(received).toBe(expected) // Object.is equality

    Expected: "Warning: Failed prop type: The prop `x` is marked as required in `inject-with-foo(C)`, but its value is `undefined`."
    Received: "Warning: Failed %s type: %s%s"

      320 |         render(<A />)
      321 |         expect(msg.length).toBe(2)
    > 322 |         expect(msg[0].split("\n")[0]).toBe(
          |                                       ^
      323 |             "Warning: Failed prop type: The prop `x` is marked as required in `inject-with-foo(C)`, but its value is `undefined`."
      324 |         )
      325 |         expect(msg[1].split("\n")[0]).toBe(

      at Object.<anonymous> (test/inject.test.tsx:322:39)

FAIL test/observer.test.tsx
  ● Console

    console.warn
      The reactive render of an observer class component (Component) 
                      was overriden after MobX attached. This may result in a memory leak if the 
                      overriden reactive render was not properly disposed.

      63 |             // Render may have been hot-swapped and/or overriden by a subclass.
      64 |             const displayName = getDisplayName(this)
    > 65 |             console.warn(
         |                     ^
      66 |                 `The reactive render of an observer class component (${displayName}) 
      67 |                 was overriden after MobX attached. This may result in a memory leak if the 
      68 |                 overriden reactive render was not properly disposed.`

      at Object.<anonymous> (node_modules/jest-mock/build/index.js:814:25)
      at Component.<anonymous> (src/observerClass.ts:65:21)
      at src/utils/utils.ts:125:20
          at Array.forEach (<anonymous>)
      at Component.wrapper (src/utils/utils.ts:124:28)

    console.warn
      The provided component class (AlreadyObserver) 
                      has already been declared as an observer component.

      25 |     if (componentClass[mobxObserverProperty]) {
      26 |         const displayName = getDisplayName(target)
    > 27 |         console.warn(
         |                 ^
      28 |             `The provided component class (${displayName}) 
      29 |                 has already been declared as an observer component.`
      30 |         )

      at Object.<anonymous> (node_modules/jest-mock/build/index.js:814:25)
      at Object.makeClassComponentObserver (src/observerClass.ts:27:17)
      at Object.observer (src/observer.tsx:57:12)
      at Object.<anonymous> (test/observer.test.tsx:881:5)

  ● nestedRendering › rerendering with outer store pop

    expect(received).toBe(expected) // Object.is equality

    Expected: undefined
    Received: [{"name": "observerTodoItem"}]

      112 |         expect(todoItemRenderings).toBe(1)
      113 |         expect(container.querySelectorAll("li").length).toBe(0)
    > 114 |         expect(getObserverTree(oldTodo, "title").observers).toBe(undefined)
          |                                                             ^
      115 |         expect(getObserverTree(oldTodo, "completed").observers).toBe(undefined)
      116 |     })
      117 | })

      at Object.<anonymous> (test/observer.test.tsx:114:61)

  ● correctly wraps display name of child component

    expect(received).toEqual(expected) // deep equality

    Expected: "StatelessObserver"
    Received: undefined

      337 | 
      338 |     expect(A.name).toEqual("ObserverClass")
    > 339 |     expect(B.displayName).toEqual("StatelessObserver")
          |                           ^
      340 | })
      341 | 
      342 | describe("124 - react to changes in this.props via computed", () => {

      at Object.<anonymous> (test/observer.test.tsx:339:27)

  ● should stop updating if error was thrown in render (#134)

    expect(received).toMatchSnapshot()

    Snapshot name: `should stop updating if error was thrown in render (#134) 1`

    - Snapshot  - 3
    + Received  + 3

      Array [
        "Error: Hello",
        Object {
          "componentStack": "
    -     in X
    -     in div (created by Outer)
    -     in Outer",
    +     at X (/Users/robertwilliams/vscode-projects/mobx-react/test/observer.test.tsx:410:9)
    +     at div
    +     at Outer (/Users/robertwilliams/vscode-projects/mobx-react/test/observer.test.tsx:393:5)",
        },
      ]

      443 |     data.set(2)
      444 |     data.set(5)
    > 445 |     expect(errors).toMatchSnapshot()
          |                    ^
      446 |     expect(lastOwnRenderCount).toBe(4)
      447 |     expect(renderingsCount).toBe(4)
      448 | })

      at Object.<anonymous> (test/observer.test.tsx:445:20)

 › 1 snapshot failed.
Snapshot Summary
 › 1 snapshot failed from 1 test suite. Inspect your code changes or re-run jest with `-u` to update them.

Test Suites: 3 failed, 10 passed, 13 total
Tests:       5 failed, 1 skipped, 101 passed, 107 total
Snapshots:   1 failed, 6 passed, 7 total
Time:        3.317s
Ran all test suites.

[mweststrate]

The problem you link to seems to be an NPM setup issue, not a compatibility at code level issue. NPM complains React@17 is not part of the peer dependency list, probably simply because that version didn't exist yet 4 years ago so it wouldn't have been able to add it there. Given your test failures above it seems however it works fine in pratice when ignoring the NPM messages (the failing tests seems to be mostly minor differences in error messages etc, if it actually wouldn't work you'd see hunderds of tests failing.

Out of curiousity though, why are you using a 4 year old version of mobx-react with React 17 in the first place?

[robhybrid]

The problem you link to seems to be an NPM setup issue, not a compatibility at code level issue. NPM complains React@17 is not part of the peer dependency list, probably simply because that version didn't exist yet 4 years ago so it wouldn't have been able to add it there. Given your test failures above it seems however it works fine in pratice when ignoring the NPM messages (the failing tests seems to be mostly minor differences in error messages etc, if it actually wouldn't work you'd see hunderds of tests failing.

Out of curiousity though, why are you using a 4 year old version of mobx-react with React 17 in the first place?

I'm not using React 17 currently. I've been developing a React/MobX application for the past 7 years, which is why I recognized this bug. I'm just trying to help resolve some bugs.

[mweststrate]

That clarifies, thanks for chiming in!