Upgrade React to 16.9

[danielkcz]

It's safe to use alpha React in dev so it's easier to write tests. Also, allow for alpha in peer deps.

cc @vkrol

[coveralls]

Coverage remained the same at 95.27% when pulling c7b81ba on upgrade-react into dbdfe3d on master.

[vkrol]

I think that using the alpha version of React is not a good idea because it can introduce some "flakiness" to the tests because is is alpha, not stable version.

#749 (comment)

[vkrol]

I'm sure we don't need to rush and we'd better wait for a stable version.

[danielkcz]

It's been alpha for several months and we are even using it in the production of two apps. It's highly likely it will be marked stable very soon. There is nothing wrong about it. And as I said, it's for dev only.

[vkrol]

Could you please replace the commit message and PR title with "Upgrade React to 16.9.0-alpha.0"? "Upgrade React" it is a little vague :(

[danielkcz]

Besides, as you can see, tests are passing and we even got the useful warning there not present with 16.8. It's not related to act, but important as well.

[vkrol]

There is nothing wrong about it. And as I said, it's for dev only.

I'm sorry, but I'll let myself disagree with that. I don't see any real reason to update yet for now. But that's my personal opinion of course.

[danielkcz]

I would agree with you if we were forcing users to upgrade as well, but we are not. It's merely about improving tests capability and ensuring we haven't missed anything future relevant.

[danielkcz]

Looks like 16.9 is really around the corner :) I am ok with waiting seeing it's so close, but it's kinda obvious it's indeed considered stable-ish already.

testing-library/react-testing-library#281 (comment)

[danielkcz]

And it's out 🎉 That was a shorter period than expected :) Since tests are passing I am going to merge this.

@vkrol Would you mind dropping those act hacks in next PR, please?

Also, those componentWillMount warnings are coming from tests if you can tackle that?

[vkrol]

@FredyC I'm gonna look at it today or tomorrow.

[vkrol]

I am not sure we should get rid of this hack. In 16.9.0 the warning message about missing act does not contain the stack trace too :(

 PASS  test/inject.test.js
  ● Console

    console.error node_modules/react-test-renderer/cjs/react-test-renderer.development.js:104
      Warning: An update to Observer inside a test was not wrapped in act(...).
      
      When testing, code that causes React state updates should be wrapped into act(...):
      
      act(() => {
        /* fire events that update state */
      });
      /* assert on the output */
      
      This ensures that you're testing the behavior the user would see in the browser. Learn more at https://fb.me/react-wrap-tests-with-act
          in Observer (created by inject-with-mapper(DisplayName))
          in inject-with-mapper(DisplayName) (created by App)
          in MobXProvider (created by App)
          in App

[danielkcz]

I see. That's a big let down. I remember stack trace being a problem in early versions of the act and expected it has improved. In that case, let's keep the hack and I will probably try to open up an issue in React about it.

[danielkcz]

Actually, why do you consider output with that hack better? Trying it now and stack trace is like that...

It just goes through that setup file to React and there is not a single clue about which test is a problem. On the contrary with an official warning which at least has DisplayName and the actual component tree which is much more information.

Edit: Well, the difference is that without that hack the test actually passes, so that's weird.

[vkrol]

Actually, why do you consider output with that hack better? Trying it now and stack trace is like that...

It's not a full stack trace. This is the full stack trace:

[mobx] Encountered an uncaught exception that was thrown by a reaction or observer component, in: 'Reaction[observer(observed)]' Error: missing act
        at Object.<anonymous>.console.error (/Users/vkrol/Projects/mobx-react/jest.setup.js:6:59)
        at warningWithoutStack (/Users/vkrol/Projects/mobx-react/node_modules/react-test-renderer/cjs/react-test-renderer.development.js:104:32)
        at warnIfNotCurrentlyActingUpdatesInDEV (/Users/vkrol/Projects/mobx-react/node_modules/react-test-renderer/cjs/react-test-renderer.development.js:14051:7)
        at dispatchAction (/Users/vkrol/Projects/mobx-react/node_modules/react-test-renderer/cjs/react-test-renderer.development.js:6467:9)
        at /Users/vkrol/Projects/mobx-react/node_modules/mobx-react-lite/dist/index.js:145:13
        at Reaction$$1.onInvalidate (/Users/vkrol/Projects/mobx-react/node_modules/mobx-react-lite/dist/index.js:169:17)
        at Reaction$$1.Object.<anonymous>.Reaction$$1.runReaction (/Users/vkrol/Projects/mobx-react/node_modules/mobx/lib/mobx.js:1646:26)
        at runReactionsHelper (/Users/vkrol/Projects/mobx-react/node_modules/mobx/lib/mobx.js:1779:35)
        at reactionScheduler (/Users/vkrol/Projects/mobx-react/node_modules/mobx/lib/mobx.js:1757:47)
        at /Users/vkrol/Projects/mobx-react/node_modules/mobx/lib/mobx.js:1786:71
        at batchedUpdates$1 (/Users/vkrol/Projects/mobx-react/node_modules/react-dom/cjs/react-dom.development.js:21643:12)
        at reactionScheduler (/Users/vkrol/Projects/mobx-react/node_modules/mobx/lib/mobx.js:1786:47)
        at runReactions$$1 (/Users/vkrol/Projects/mobx-react/node_modules/mobx/lib/mobx.js:1762:5)
        at endBatch$$1 (/Users/vkrol/Projects/mobx-react/node_modules/mobx/lib/mobx.js:1469:9)
        at ObservableValue$$1.Object.<anonymous>.Atom$$1.reportChanged (/Users/vkrol/Projects/mobx-react/node_modules/mobx/lib/mobx.js:250:9)
        at ObservableValue$$1.Object.<anonymous>.ObservableValue$$1.setNewValue (/Users/vkrol/Projects/mobx-react/node_modules/mobx/lib/mobx.js:745:14)
        at ObservableObjectAdministration$$1.Object.<anonymous>.ObservableObjectAdministration$$1.write (/Users/vkrol/Projects/mobx-react/node_modules/mobx/lib/mobx.js:3862:27)
        at set$$1 (/Users/vkrol/Projects/mobx-react/node_modules/mobx/lib/mobx.js:2458:17)
        at Object.set (/Users/vkrol/Projects/mobx-react/node_modules/mobx/lib/mobx.js:2782:9)
--->    at Object.test (/Users/vkrol/Projects/mobx-react/test/inject.test.js:423:13)
        at Object.asyncJestTest (/Users/vkrol/Projects/mobx-react/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:102:37)
        at resolve (/Users/vkrol/Projects/mobx-react/node_modules/jest-jasmine2/build/queueRunner.js:41:12)
        at new Promise (<anonymous>)
        at mapper (/Users/vkrol/Projects/mobx-react/node_modules/jest-jasmine2/build/queueRunner.js:26:19)
        at promise.then (/Users/vkrol/Projects/mobx-react/node_modules/jest-jasmine2/build/queueRunner.js:71:41)

[vkrol]

Edit: Well, the difference is that without that hack the test actually passes, so that's weird.

It's not weird because React do not throw about missing act, only warn.

[danielkcz]

Oh wow, that's hardly helpful. I would not have been looking for like 20th line in a stack trace 😮 But ok, let's keep it and we can at least filter that out and find the first line which does not contain node_modules and jest.setup.js.

[danielkcz]

I've open an issue with React: facebook/react#16348
There is also a link to sandbox with the filtering solution I've talked about.

[vkrol]

@FredyC awesome, thanks!