Connect to store before children connect to the store (#31)

Connecting to the store in didMount means that child components connect to the store before their parent component. This causes them to render before their parent component which can lead to issues if the state they need depends on their props and is no longer valid due to the store changing. 

The specific problem I have is that when a Player leaves, the in game playerlist has errors because the children are updated before their parent. The child component tries to access information that no longer exists in the store, but the parent will stop rendering this child when it updates due to the player leaving anyway.
I think we should be able to fix this by changing it so that connecting to the store in RoactRodux is done in init, not didMount since init is called first for parents and then for children whereas didMount is called for children first. 

We could also fix this more explicitly by connecting to the store by priority (deeper in the tree = less priority). This might be a possible future fix to the problem.

I added configuration for this change by copying the GlobalConfig pattern from Roact. Let me know if I should do something a bit more lightweight than this instead.

Author: ConorGriffin37

CHANGELOG.md

@@ -1,7 +1,7 @@
 # RoactRodux Changelog
 
 ## Current master
-* No changes
+* Change order of connection to store so that Parent components are connected to the store before their children.
 
 ## 1.0.0 (TODO: Date)
 * Initial release

src/TempConfig.lua

@@ -0,0 +1,3 @@
+return {
+	newConnectionOrder = true,
+}

src/connect.lua

@@ -3,6 +3,8 @@ local getStore = require(script.Parent.getStore)
 local shallowEqual = require(script.Parent.shallowEqual)
 local join = require(script.Parent.join)
 
+local TempConfig = require(script.Parent.TempConfig)
+
 --[[
 	Formats a multi-line message with printf-style placeholders.
 ]]
@@ -85,6 +87,23 @@ local function connect(mapStateToPropsOrThunk, mapDispatchToProps)
 			end
 		end
 
+		function Connection:createStoreConnection()
+			self.storeChangedConnection = self.store.changed:connect(function(storeState)
+				self:setState(function(prevState, props)
+					local mappedStoreState = prevState.mapStateToProps(storeState, props)
+
+					-- We run this check here so that we only check shallow
+					-- equality with the result of mapStateToProps, and not the
+					-- other props that could be passed through the connector.
+					if shallowEqual(mappedStoreState, prevState.mappedStoreState) then
+						return nil
+					end
+
+					return prevState.stateUpdater(props, prevState, mappedStoreState)
+				end)
+			end)
+		end
+
 		function Connection:init()
 			self.store = getStore(self)
 
@@ -155,23 +174,16 @@ local function connect(mapStateToPropsOrThunk, mapDispatchToProps)
 			for key, value in pairs(extraState) do
 				self.state[key] = value
 			end
+
+			if TempConfig.newConnectionOrder then
+				self:createStoreConnection()
+			end
 		end
 
 		function Connection:didMount()
-			self.storeChangedConnection = self.store.changed:connect(function(storeState)
-				self:setState(function(prevState, props)
-					local mappedStoreState = prevState.mapStateToProps(storeState, props)
-
-					-- We run this check here so that we only check shallow
-					-- equality with the result of mapStateToProps, and not the
-					-- other props that could be passed through the connector.
-					if shallowEqual(mappedStoreState, prevState.mappedStoreState) then
-						return nil
-					end
-
-					return prevState.stateUpdater(props, prevState, mappedStoreState)
-				end)
-			end)
+			if not TempConfig.newConnectionOrder then
+				self:createStoreConnection()
+			end
 		end
 
 		function Connection:willUnmount()

src/connect.spec.lua

@@ -6,6 +6,8 @@ return function()
 	local Roact = require(script.Parent.Parent.Roact)
 	local Rodux = require(script.Parent.Parent.Rodux)
 
+	local TempConfig = require(script.Parent.TempConfig)
+
 	local function noop()
 		return nil
 	end
@@ -248,4 +250,104 @@ return function()
 
 		Roact.unmount(handle)
 	end)
+
+	it("should render parent elements before children", function()
+		local oldNewConnectionOrder = TempConfig.newConnectionOrder
+		TempConfig.newConnectionOrder = true
+
+		local function mapStateToProps(state)
+			return {
+				count = state.count,
+			}
+		end
+
+		local childWasRenderedFirst = false
+
+		local function ChildComponent(props)
+			if props.count > props.parentCount then
+				childWasRenderedFirst = true
+			end
+		end
+
+		local ConnectedChildComponent = connect(mapStateToProps)(ChildComponent)
+
+		local function ParentComponent(props)
+			return Roact.createElement(ConnectedChildComponent, {
+				parentCount = props.count,
+			})
+		end
+
+		local ConnectedParentComponent = connect(mapStateToProps)(ParentComponent)
+
+		local store = Rodux.Store.new(reducer)
+		local tree = Roact.createElement(StoreProvider, {
+			store = store,
+		}, {
+			parent = Roact.createElement(ConnectedParentComponent),
+		})
+
+		local handle = Roact.mount(tree)
+
+		store:dispatch({ type = "increment" })
+		store:flush()
+
+		store:dispatch({ type = "increment" })
+		store:flush()
+
+		Roact.unmount(handle)
+
+		expect(childWasRenderedFirst).to.equal(false)
+
+		TempConfig.newConnectionOrder = oldNewConnectionOrder
+	end)
+
+	it("should render child elements before children when TempConfig.newConnectionOrder is false", function()
+		local oldNewConnectionOrder = TempConfig.newConnectionOrder
+		TempConfig.newConnectionOrder = false
+
+		local function mapStateToProps(state)
+			return {
+				count = state.count,
+			}
+		end
+
+		local childWasRenderedFirst = false
+
+		local function ChildComponent(props)
+			if props.count > props.parentCount then
+				childWasRenderedFirst = true
+			end
+		end
+
+		local ConnectedChildComponent = connect(mapStateToProps)(ChildComponent)
+
+		local function ParentComponent(props)
+			return Roact.createElement(ConnectedChildComponent, {
+				parentCount = props.count,
+			})
+		end
+
+		local ConnectedParentComponent = connect(mapStateToProps)(ParentComponent)
+
+		local store = Rodux.Store.new(reducer)
+		local tree = Roact.createElement(StoreProvider, {
+			store = store,
+		}, {
+			parent = Roact.createElement(ConnectedParentComponent),
+		})
+
+		local handle = Roact.mount(tree)
+
+		store:dispatch({ type = "increment" })
+		store:flush()
+
+		store:dispatch({ type = "increment" })
+		store:flush()
+
+		Roact.unmount(handle)
+
+		expect(childWasRenderedFirst).to.equal(true)
+
+		TempConfig.newConnectionOrder = oldNewConnectionOrder
+	end)
 end

src/init.lua

@@ -1,10 +1,13 @@
 local StoreProvider = require(script.StoreProvider)
 local connect = require(script.connect)
 local getStore = require(script.getStore)
+local TempConfig = require(script.TempConfig)
 
 return {
 	StoreProvider = StoreProvider,
 	connect = connect,
 	UNSTABLE_getStore = getStore,
 	UNSTABLE_connect2 = connect,
+
+	TEMP_CONFIG = TempConfig,
 }

test-place.project.json

@@ -15,7 +15,7 @@
       },
 
       "Rodux": {
-        "$path": "modules/rodux/lib"
+        "$path": "modules/rodux/src"
       },
 
       "TestEZ": {

test/lemur.lua

@@ -5,7 +5,7 @@
 -- If you add any dependencies, add them to this table so they'll be loaded!
 local LOAD_MODULES = {
 	{"src", "RoactRodux"},
-	{"modules/rodux/lib", "Rodux"},
+	{"modules/rodux/src", "Rodux"},
 	{"modules/roact/src", "Roact"},
 	{"modules/testez/lib", "TestEZ"},
 }