Provider: Forbid changing stores

CHANGELOG.md

@@ -8,6 +8,7 @@
 * Killed the possibility to directly pass store names to `observer`. Always use `inject` instead. (This was deprecate for a long time already). `observer(["a", "b"], component)` should now be written as `inject("a", "b")(component)`.
 * `observer` components no longer automatically recover from errors (to prevent potential memory leaks). Instead, this is the responsibility of error boundaries.
 * `inject` now supports ref forwarding. As such, the `.wrappedInstance` property has been removed since refs can be used instead. (Fixes [#616](https://github.com/mobxjs/mobx-react/issues/616) (See also [#619](https://github.com/mobxjs/mobx-react/pull/619) by [42shadow42](https://github.com/42shadow42))
+* Changing the set of stores in `Provider` is no longer supported and while throw a hard error (this was a warning before), as the model of `Provider` / `inject` has always been to inject final values into the tree. (That is, fixed references, injected objects themselves can be stateful without problem). If you want to dynamically swap what is provided into the tree, use `React.createContext` instead of `Provider` / `inject`. The  suppressChangedStoreWarning` flag for `Provider` has been dropped.
 
 **Improvements**
 

src/Provider.js

@@ -1,5 +1,5 @@
 import { Children, Component, createContext, createElement } from "react"
-import * as PropTypes from "./propTypes"
+import { shallowEqual } from "./utils/utils"
 
 const specialReactKeys = { children: true, key: true, ref: true }
 
@@ -35,23 +35,12 @@ class Provider extends Component {
         if (!nextProps) return null
         if (!prevState) return nextProps
 
-        // Maybe this warning is too aggressive?
-        if (
-            Object.keys(nextProps).filter(validStoreName).length !==
-            Object.keys(prevState).filter(validStoreName).length
-        )
-            console.warn(
+        const baseStores = { ...prevState, ...specialReactKeys } // mix in specialReactKeys, so that they are ignored in the diff
+        const newStores = { ...nextProps, ...specialReactKeys }
+        if (!shallowEqual(baseStores, newStores))
+            throw new Error(
                 "MobX Provider: The set of provided stores has changed. Please avoid changing stores as the change might not propagate to all children"
             )
-        if (!nextProps.suppressChangedStoreWarning)
-            for (let key in nextProps)
-                if (validStoreName(key) && prevState[key] !== nextProps[key])
-                    console.warn(
-                        "MobX Provider: Provided store '" +
-                            key +
-                            "' has changed. Please avoid replacing stores as the change might not propagate to all children"
-                    )
-
         return nextProps
     }
 }
@@ -64,5 +53,4 @@ function copyStores(from, to) {
 function validStoreName(key) {
     return !specialReactKeys[key] && key !== "suppressChangedStoreWarning"
 }
-
 export default Provider

test/__snapshots__/inject.test.js.snap

@@ -0,0 +1,8 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`inject based context warning is printed when changing stores 1`] = `
+Array [
+  "Error: Uncaught [Error: MobX Provider: The set of provided stores has changed. Please avoid changing stores as the change might not propagate to all children]",
+  "The above error occurred in the <Provider> component:",
+]
+`;

test/inject.test.js

@@ -168,9 +168,9 @@ describe("inject based context", () => {
     })
 
     test("warning is printed when changing stores", () => {
-        let msg
-        const baseWarn = console.warn
-        console.warn = m => (msg = m)
+        let msgs = []
+        const baseError = console.error
+        console.error = m => msgs.push(m.split("\n")[0]) // drop stacktraces to avoid line number issues
         const a = mobx.observable.box(3)
         const C = inject("foo")(
             observer(
@@ -208,15 +208,14 @@ describe("inject based context", () => {
         expect(wrapper.find("span").text()).toBe("3")
         expect(wrapper.find("div").text()).toBe("context:3")
 
-        a.set(42)
-
-        expect(wrapper.find("span").text()).toBe("42")
-        expect(wrapper.find("div").text()).toBe("context:3")
-        expect(msg).toBe(
-            "MobX Provider: Provided store 'foo' has changed. Please avoid replacing stores as the change might not propagate to all children"
+        expect(() => {
+            a.set(42)
+        }).toThrow(
+            "The set of provided stores has changed. Please avoid changing stores as the change might not propagate to all children"
         )
+        expect(msgs).toMatchSnapshot() // nobody caught the error of the rendering
 
-        console.warn = baseWarn
+        console.error = baseError
     })
 
     test("custom storesToProps", () => {