Recover from bad inputs (#60)

Store and Signal will now catch errors that are thrown when reducers are firing and will prevent the Store from falling apart entirely when errors occur. Store.new now takes an optional error reporter interface, and defaults to printing.

This is a breaking change, as the previous behavior was to propagate the error rather than print it.

Author: matthargett

.luacheckrc

@@ -24,6 +24,7 @@ stds.roblox = {
 stds.testez = {
 	read_globals = {
 		"describe",
+		"beforeEach", "afterEach", "beforeAll", "afterAll",
 		"it", "itFOCUS", "itSKIP",
 		"FOCUS", "SKIP", "HACK_NO_XPCALL",
 		"expect",

CHANGELOG.md

@@ -1,6 +1,7 @@
 # Rodux Changelog
 
 ## Unreleased Changes
+* Introduce error handling to catch and report errors during reducers ([#60](https://github.com/Roblox/rodux/pull/60)).
 
 ## 1.1.0 (2021-01-04)
 * Added color schemes for documentation based on user preference ([#56](https://github.com/Roblox/rodux/pull/56)).

modules/lemur

@@ -5,7 +5,7 @@
 -- If you add any dependencies, add them to this table so they'll be loaded!
 local LOAD_MODULES = {
 	{"src", "Library"},
-	{"modules/testez/lib", "TestEZ"},
+	{"modules/testez/src", "TestEZ"},
 }
 
 -- This makes sure we can load Lemur and other libraries that depend on init.lua
@@ -31,7 +31,10 @@ end
 -- Load TestEZ and run our tests
 local TestEZ = habitat:require(Root.TestEZ)
 
-local results = TestEZ.TestBootstrap:run(Root.Library, TestEZ.Reporters.TextReporter)
+local results = TestEZ.TestBootstrap:run(
+	{ Root.Library },
+	TestEZ.Reporters.TextReporter
+)
 
 -- Did something go wrong?
 if results.failureCount > 0 then

modules/testez

@@ -1,3 +1,5 @@
+--!nocheck
+
 --[[
 	Calls a function and throws an error if it attempts to yield.
 
@@ -26,4 +28,4 @@ local function NoYield(callback, ...)
 	return resultHandler(co, coroutine.resume(co, ...))
 end
 
-return NoYield
+return NoYield

spec.lua

@@ -4,6 +4,7 @@
 	Handlers are fired in order, and (dis)connections are properly handled when
 	executing an event.
 ]]
+local inspect = require(script.Parent.inspect).inspect
 
 local function immutableAppend(list, ...)
 	local new = {}
@@ -36,9 +37,10 @@ local Signal = {}
 
 Signal.__index = Signal
 
-function Signal.new()
+function Signal.new(store)
 	local self = {
-		_listeners = {}
+		_listeners = {},
+		_store = store
 	}
 
 	setmetatable(self, Signal)
@@ -47,15 +49,45 @@ function Signal.new()
 end
 
 function Signal:connect(callback)
+	if typeof(callback) ~= "function" then
+		error("Expected the listener to be a function.")
+	end
+
+	if self._store and self._store._isDispatching then
+		error(
+			'You may not call store.changed:connect() while the reducer is executing. ' ..
+			  'If you would like to be notified after the store has been updated, subscribe from a ' ..
+			  'component and invoke store:getState() in the callback to access the latest state. '
+		)
+	end
+
 	local listener = {
 		callback = callback,
 		disconnected = false,
+		connectTraceback = debug.traceback(),
+		disconnectTraceback = nil
 	}
 
 	self._listeners = immutableAppend(self._listeners, listener)
 
 	local function disconnect()
+		if listener.disconnected then
+			local errorMessage = ("Listener connected at: \n%s\n" ..
+				"was already disconnected at: \n%s\n"):format(
+					tostring(listener.connectTraceback),
+					tostring(listener.disconnectTraceback)
+			)
+			self._store._errorReporter:reportErrorDeferred(errorMessage, debug.traceback())
+
+			return
+		end
+
+		if self._store and self._store._isDispatching then
+			error("You may not unsubscribe from a store listener while the reducer is executing.")
+		end
+
 		listener.disconnected = true
+		listener.disconnectTraceback = debug.traceback()
 		self._listeners = immutableRemoveValue(self._listeners, listener)
 	end
 
@@ -64,10 +96,35 @@ function Signal:connect(callback)
 	}
 end
 
+function Signal:reportListenerError(listener, callbackArgs, error_)
+	local message = ("Caught error when calling event listener (%s), " ..
+		"originally subscribed from: \n%s\n" ..
+		"with arguments: \n%s\n"):format(
+			tostring(listener.callback),
+			tostring(listener.connectTraceback),
+			inspect(callbackArgs)
+		)
+
+	if self._store then
+		self._store._errorReporter:reportErrorImmediately(message, error_)
+	else
+		print(message .. tostring(error_))
+	end
+end
+
 function Signal:fire(...)
 	for _, listener in ipairs(self._listeners) do
 		if not listener.disconnected then
-			listener.callback(...)
+			local ok, result = pcall(function(...)
+				listener.callback(...)
+			end, ...)
+			if not ok then
+				self:reportListenerError(
+					listener,
+					{...},
+					result
+				)
+			end
 		end
 	end
 end

src/NoYield.lua

@@ -111,4 +111,85 @@ return function()
 		expect(countA).to.equal(1)
 		expect(countB).to.equal(0)
 	end)
+
+	describe("when event handlers error", function()
+		local reportedErrorError, reportedErrorMessage
+		local mockStore = {
+			_errorReporter = {
+				reportErrorImmediately = function(_self, message, error_)
+					reportedErrorMessage = message
+					reportedErrorError = error_
+				end,
+				reportErrorDeferred = function(_self, message, error_)
+					reportedErrorMessage = message
+					reportedErrorError = error_
+				end
+			}
+		}
+
+		beforeEach(function()
+			reportedErrorError = ""
+			reportedErrorMessage = ""
+		end)
+
+		it("first listener succeeds when second listener errors", function()
+			local signal = Signal.new(mockStore)
+			local countA = 0
+
+			signal:connect(function()
+				countA = countA + 1
+			end)
+
+			signal:connect(function()
+				error("connectionB")
+			end)
+
+			signal:fire()
+
+			expect(countA).to.equal(1)
+			local caughtErrorMessage = "Caught error when calling event listener"
+			expect(string.find(reportedErrorMessage, caughtErrorMessage)).to.be.ok()
+			local caughtErrorError = "connectionB"
+			expect(string.find(reportedErrorError, caughtErrorError)).to.be.ok()
+		end)
+
+		it("second listener succeeds when first listener errors", function()
+			local signal = Signal.new(mockStore)
+			local countB = 0
+
+			signal:connect(function()
+				error("connectionA")
+			end)
+
+			signal:connect(function()
+				countB = countB + 1
+			end)
+
+			signal:fire()
+
+			expect(countB).to.equal(1)
+			local caughtErrorMessage = "Caught error when calling event listener"
+			expect(string.find(reportedErrorMessage, caughtErrorMessage)).to.be.ok()
+			local caughtErrorError = "connectionA"
+			expect(string.find(reportedErrorError, caughtErrorError)).to.be.ok()
+		end)
+
+		it("serializes table arguments when reporting errors", function()
+			local signal = Signal.new(mockStore)
+
+			signal:connect(function()
+				error("connectionA")
+			end)
+
+			local actionCommand = "SENTINEL"
+			signal:fire({actionCommand = actionCommand})
+
+			local caughtErrorMessage = "Caught error when calling event listener"
+			local caughtErrorArg = "actionCommand: \"" .. actionCommand .. "\""
+			expect(string.find(reportedErrorMessage, caughtErrorMessage)).to.be.ok()
+			expect(string.find(reportedErrorMessage, caughtErrorArg)).to.be.ok()
+			local caughtErrorError = "connectionA"
+			expect(string.find(reportedErrorError, caughtErrorError)).to.be.ok()
+		end)
+	end)
 end

src/Signal.lua

@@ -2,6 +2,18 @@ local RunService = game:GetService("RunService")
 
 local Signal = require(script.Parent.Signal)
 local NoYield = require(script.Parent.NoYield)
+local inspect = require(script.Parent.inspect).inspect
+
+local defaultErrorReporter = {
+	reportErrorDeferred = function(self, message, stacktrace)
+		print(message)
+		print(stacktrace)
+	end,
+	reportErrorImmediately = function(self, message, stacktrace)
+		print(message)
+		print(stacktrace)
+	end
+}
 
 local Store = {}
 
@@ -23,22 +35,41 @@ Store.__index = Store
 	Reducers do not mutate the state object, so the original state is still
 	valid.
 ]]
-function Store.new(reducer, initialState, middlewares)
+function Store.new(reducer, initialState, middlewares, errorReporter)
 	assert(typeof(reducer) == "function", "Bad argument #1 to Store.new, expected function.")
 	assert(middlewares == nil or typeof(middlewares) == "table", "Bad argument #3 to Store.new, expected nil or table.")
+	if middlewares ~= nil then
+		for i=1, #middlewares, 1 do
+			assert(
+				typeof(middlewares[i]) == "function",
+				("Expected the middleware ('%s') at index %d to be a function."):format(tostring(middlewares[i]), i)
+			)
+		end
+	end
 
 	local self = {}
 
+	self._errorReporter = errorReporter or defaultErrorReporter
+	self._isDispatching = false
 	self._reducer = reducer
-	self._state = reducer(initialState, {
+	local initAction = {
 		type = "@@INIT",
-	})
+	}
+	self._lastAction = initAction
+	local ok, result = pcall(function()
+		self._state = reducer(initialState, initAction)
+	end)
+	if not ok then
+		local message = ("Caught error with init action of reducer (%s): %s"):format(tostring(reducer), tostring(result))
+		errorReporter:reportErrorImmediately(message, debug.traceback())
+		self._state = initialState
+	end
 	self._lastState = self._state
 
 	self._mutatedSinceFlush = false
 	self._connections = {}
 
-	self.changed = Signal.new()
+	self.changed = Signal.new(self)
 
 	setmetatable(self, Store)
 
@@ -58,7 +89,7 @@ function Store.new(reducer, initialState, middlewares)
 			dispatch = middleware(dispatch, self)
 		end
 
-		self.dispatch = function(self, ...)
+		self.dispatch = function(_self, ...)
 			return dispatch(...)
 		end
 	end
@@ -70,9 +101,29 @@ end
 	Get the current state of the Store. Do not mutate this!
 ]]
 function Store:getState()
+	if self._isDispatching then
+		error(("You may not call store:getState() while the reducer is executing. " ..
+			"The reducer (%s) has already received the state as an argument. " ..
+			"Pass it down from the top reducer instead of reading it from the store."):format(tostring(self._reducer)))
+	end
+
 	return self._state
 end
 
+function Store:_reportReducerError(failedAction, error_, traceback)
+	local message = ("Caught error when running action (%s) " ..
+		"through reducer (%s): \n%s \n" ..
+		"previous action type was: %s"
+	):format(
+		tostring(failedAction),
+		tostring(self._reducer),
+		tostring(error_),
+		inspect(self._lastAction)
+	)
+
+	self._errorReporter:reportErrorImmediately(message, traceback)
+end
+
 --[[
 	Dispatch an action to the store. This allows the store's reducer to mutate
 	the state of the application by creating a new copy of the state.
@@ -81,16 +132,39 @@ end
 	changes, but not necessarily on every Dispatch.
 ]]
 function Store:dispatch(action)
-	if typeof(action) == "table" then
-		if action.type == nil then
-			error("action does not have a type field", 2)
-		end
+	if typeof(action) ~= "table" then
+		error(("Actions must be tables. " ..
+			"Use custom middleware for %q actions."):format(typeof(action)),
+			2
+		)
+	end
 
+	if action.type == nil then
+		error("Actions may not have an undefined 'type' property. " ..
+			"Have you misspelled a constant? \n" ..
+			inspect(action), 2)
+	end
+
+	if self._isDispatching then
+		error("Reducers may not dispatch actions.")
+	end
+
+	local ok, result = pcall(function()
+		self._isDispatching = true
 		self._state = self._reducer(self._state, action)
 		self._mutatedSinceFlush = true
-	else
-		error(("actions of type %q are not permitted"):format(typeof(action)), 2)
+	end)
+
+	self._isDispatching = false
+
+	if not ok then
+		self:_reportReducerError(
+			action,
+			result,
+			debug.traceback()
+		)
 	end
+	self._lastAction = action
 end
 
 --[[