Better error reporter interface #61
Conversation
…flush/update errors
… of the reporters
| error("Actions may not have an undefined 'type' property. " .. | ||
| "Have you misspelled a constant? \n" .. | ||
| inspect(action), 2) | ||
| tostring(action), 2) |
There was a problem hiding this comment.
is tostring() going to give actiomable information when it’s a table with the wrong format?
There was a problem hiding this comment.
Maybe not! But this is the kind of error that we're much more likely to catch in dev (it was an error in 1.x before). It didn't seem like it justified all of the inspect logic, some of which actually had to be adjusted to work in non-Luau contexts for the sake of this repo's CI
There was a problem hiding this comment.
Is it possible for us to safely get action.name here? That would be more useful than the pointer/id.
There was a problem hiding this comment.
Not sure I follow; action.name doesn't have any particular meaning. The only required structure for an action is that it's a table with a type field (maybe that's what you're thinking of?). This error will only be thrown in the event that an action without a type is dispatched.
I think the way this error happens in practice is if you accidentally dispatch something other than a proper action in your mapDispatchToProps, which would make whatever you were trying to do fail altogether. Hence why I suggested that this is likely an error that folks find and resolve during development.
There was a problem hiding this comment.
Sorry, action.type is the correct nomenclature. In the Lua App codebase we have a "name" argument (ultimately Script.name) that gets assigned to "type" in Action.lua. Makes things a little confusing.
I'm just thinking that it will provide a nicer error message and doesn't require you to go through the expense of visiting the whole table. Not a big deal.
…e, and test behavior
| message = "Caught error in reducer with init", | ||
| thrownValue = result, | ||
| }) | ||
| self._state = initialState |
There was a problem hiding this comment.
I know this is from previous code, but is this state update logic ok? If "not ok then self._state and self._lastState both equal initialState"? If we're resetting, should lastState be the previous lastState?
There was a problem hiding this comment.
It shouldn't be an issue; technically, both things are true. Your last state was the initial state passed into Store.new; your current state is also that, because your reducer threw an error.
| error("Actions may not have an undefined 'type' property. " .. | ||
| "Have you misspelled a constant? \n" .. | ||
| inspect(action), 2) | ||
| tostring(action), 2) |
There was a problem hiding this comment.
Is it possible for us to safely get action.name here? That would be more useful than the pointer/id.
matthargett
left a comment
matthargett
left a comment
There was a problem hiding this comment.
great job on the test coverage!
src/Store.lua
Outdated
| end | ||
| } | ||
|
|
||
| local tracebackReporter = function(message) |
There was a problem hiding this comment.
| local tracebackReporter = function(message) | |
| local function tracebackReporter(message) |
|
|
||
| errorReporter:reportErrorImmediately(message, traceback) | ||
| local function tracebackReporter(message) | ||
| return debug.traceback(message) |
There was a problem hiding this comment.
In case an non-string error gets thrown, should we tostring it here?
There was a problem hiding this comment.
Hmm that's a good idea, yeah
src/Store.spec.lua
Outdated
| it("should throw errors if dispatching while a dispatch is already happening", function() | ||
| local store | ||
| store = Store.new(function(state, action) | ||
| if action.type ~= "@@INIT" then |
There was a problem hiding this comment.
Little detail but I think it'd be a little clearer if the action type matched the dispatched one
| if action.type ~= "@@INIT" then | |
| if action.type == "SomeAction" then |
4 participants
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:
deferredandimmediateabstraction from external codebases. Instead, categorize errors into ones that occur when processing actions withreportReducerError(running reducers on dispatched actions) and ones that occur when updating listeners withreportUpdateError(flushing new state)Remaining work: