Merge 212921e into c9b9807

Roblox · Mar 25, 2021 · 18bc84a · 18bc84a
2 parents c9b9807 + 212921e
commit 18bc84a
Show file tree

Hide file tree

Showing 9 changed files with 260 additions and 400 deletions.
diff --git a/src/Signal.lua b/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
@@ -72,12 +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())
+			error((
+				"Listener connected at: \n%s\n" ..
+				"was already disconnected at: \n%s\n"
+			):format(
+				tostring(listener.connectTraceback),
+				tostring(listener.disconnectTraceback)
+			))
 
 			return
 		end
@@ -96,35 +95,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

diff --git a/src/Signal.spec.lua b/src/Signal.spec.lua
@@ -111,85 +111,4 @@ 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
diff --git a/src/Store.lua b/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 tracebackReporter = function(message)
+	return debug.traceback(message, 3)
+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