Better error reporter interface (#61)

We were a bit hasty with the changes that were made to v2.0.0! It seems prudent to circle back and make a few improvements:

Reverts the default behavior to throwing the error instead of printing it (this aligns better with expectations from Rodux 1.x)
Don't inherit the deferred and immediate abstraction from external codebases. Instead, categorize errors into ones that occur when processing actions with reportReducerError (running reducers on dispatched actions) and ones that occur when updating listeners with reportUpdateError (flushing new state)
For the error reporter interface, provide a set of data relevant to each situation. This includes recent actions, store state, and the thrown error
Removes error reporting from signal, to try to keep it simple and generic; most usage of rodux is through helper libraries that manage the signal connections, anyway.
Updates tests to account for new expected behavior
Carves out some overzealous printing logic; users of the error reporter API can opt into it isntead

Author: ZoteTheMighty

src/Signal.lua

@@ -4,8 +4,6 @@
 	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 = {}
 	local len = #list
@@ -56,8 +54,8 @@ function Signal:connect(callback)
 	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. '
+				'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
 
@@ -72,14 +70,13 @@ function Signal:connect(callback)
 
 	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
+			error((
+				"Listener connected at: \n%s\n" ..
+				"was already disconnected at: \n%s\n"
+			):format(
+				tostring(listener.connectTraceback),
+				tostring(listener.disconnectTraceback)
+			))
 		end
 
 		if self._store and self._store._isDispatching then
@@ -96,35 +93,10 @@ 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
-			local ok, result = pcall(function(...)
-				listener.callback(...)
-			end, ...)
-			if not ok then
-				self:reportListenerError(
-					listener,
-					{...},
-					result
-				)
-			end
+			listener.callback(...)
 		end
 	end
 end

src/Signal.spec.lua

@@ -112,84 +112,55 @@ return function()
 		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("should throw an error if the argument to `connect` is not a function", function()
+		local signal = Signal.new()
+		expect(function()
+			signal:connect("not a function")
+		end).to.throw()
+	end)
 
-		it("first listener succeeds when second listener errors", function()
-			local signal = Signal.new(mockStore)
-			local countA = 0
+	it("should throw an error when disconnecting more than once", function()
+		local signal = Signal.new()
 
-			signal:connect(function()
-				countA = countA + 1
-			end)
+		local connection = signal:connect(function() end)
+		-- Okay to disconnect once
+		expect(connection.disconnect).never.to.throw()
 
-			signal:connect(function()
-				error("connectionB")
-			end)
+		-- Throw an error if we disconnect twice
+		expect(connection.disconnect).to.throw()
+	end)
 
-			signal:fire()
+	it("should throw an error when subscribing during dispatch", function()
+		local mockStore = {
+			_isDispatching = false
+		}
+		local signal = Signal.new(mockStore)
 
-			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()
+		signal:connect(function()
+			-- Subscribe while listeners are being fired
+			signal:connect(function() end)
 		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)
-
+		mockStore._isDispatching = true
+		expect(function()
 			signal:fire()
+		end).to.throw()
+	end)
 
-			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})
+	it("should throw an error when unsubscribing during dispatch", function()
+		local mockStore = {
+			_isDispatching = false
+		}
+		local signal = Signal.new(mockStore)
 
-			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()
+		local connection
+		connection = signal:connect(function()
+			connection.disconnect()
 		end)
+
+		mockStore._isDispatching = true
+		expect(function()
+			signal:fire()
+		end).to.throw()
 	end)
 end

src/Store.lua

@@ -2,19 +2,22 @@ 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)
+local ACTION_LOG_LENGTH = 3
+
+local rethrowErrorReporter = {
+	reportReducerError = function(prevState, action, errorResult)
+		error(string.format("Received error: %s\n\n%s", errorResult.message, errorResult.thrownValue))
+	end,
+	reportUpdateError = function(prevState, currentState, lastActions, errorResult)
+		error(string.format("Received error: %s\n\n%s", errorResult.message, errorResult.thrownValue))
 	end,
-	reportErrorImmediately = function(self, message, stacktrace)
-		print(message)
-		print(stacktrace)
-	end
 }
 
+local function tracebackReporter(message)
+	return debug.traceback(tostring(message))
+end
+
 local Store = {}
 
 -- This value is exposed as a private value so that the test code can stay in
@@ -49,19 +52,21 @@ function Store.new(reducer, initialState, middlewares, errorReporter)
 
 	local self = {}
 
-	self._errorReporter = errorReporter or defaultErrorReporter
+	self._errorReporter = errorReporter or rethrowErrorReporter
 	self._isDispatching = false
 	self._reducer = reducer
 	local initAction = {
 		type = "@@INIT",
 	}
-	self._lastAction = initAction
-	local ok, result = pcall(function()
+	self._actionLog = { initAction }
+	local ok, result = xpcall(function()
 		self._state = reducer(initialState, initAction)
-	end)
+	end, tracebackReporter)
 	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._errorReporter.reportReducerError(initialState, initAction, {
+			message = "Caught error in reducer with init",
+			thrownValue = result,
+		})
 		self._state = initialState
 	end
 	self._lastState = self._state
@@ -110,20 +115,6 @@ function Store:getState()
 	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.
@@ -142,7 +133,7 @@ function Store:dispatch(action)
 	if action.type == nil then
 		error("Actions may not have an undefined 'type' property. " ..
 			"Have you misspelled a constant? \n" ..
-			inspect(action), 2)
+			tostring(action), 2)
 	end
 
 	if self._isDispatching then
@@ -158,13 +149,20 @@ function Store:dispatch(action)
 	self._isDispatching = false
 
 	if not ok then
-		self:_reportReducerError(
+		self._errorReporter.reportReducerError(
+			self._state,
 			action,
-			result,
-			debug.traceback()
+			{
+				message = "Caught error in reducer",
+				thrownValue = result,
+			}
 		)
 	end
-	self._lastAction = action
+
+	if #self._actionLog == ACTION_LOG_LENGTH then
+		table.remove(self._actionLog, 1)
+	end
+	table.insert(self._actionLog, action)
 end
 
 --[[
@@ -193,11 +191,25 @@ function Store:flush()
 	-- unless we cache this value first
 	local state = self._state
 
-	-- If a changed listener yields, *very* surprising bugs can ensue.
-	-- Because of that, changed listeners cannot yield.
-	NoYield(function()
-		self.changed:fire(state, self._lastState)
-	end)
+	local ok, errorResult = xpcall(function()
+		-- If a changed listener yields, *very* surprising bugs can ensue.
+		-- Because of that, changed listeners cannot yield.
+		NoYield(function()
+			self.changed:fire(state, self._lastState)
+		end)
+	end, tracebackReporter)
+
+	if not ok then
+		self._errorReporter.reportUpdateError(
+			self._lastState,
+			state,
+			self._actionLog,
+			{
+				message = "Caught error flushing store updates",
+				thrownValue = errorResult,
+			}
+		)
+	end
 
 	self._lastState = state
 end