[core] feat(interpreter): allow to provide errorListeners#2841
[core] feat(interpreter): allow to provide errorListeners#2841roggervalf wants to merge 17 commits intostatelyai:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 865f2ce The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
CodeSee Review Map:Review in an interactive map View more CodeSee Maps Legend |
packages/core/src/interpreter.ts
Outdated
| if (this.errorListeners.size) { | ||
| this.sendError(error); | ||
| } |
There was a problem hiding this comment.
There are more places where errors could happen and I think they should be covered as well IF we decide to add this feature.
I think that in general there might be a need for a top-level onError listener but we should think carefully through how this works with multiple nested machines etc.
There was a problem hiding this comment.
thank you @Andarist, I tried to update my pr, please when you get a chance 👀 to take a look
There was a problem hiding this comment.
There are more places where errors could happen and I think they should be covered as well IF we decide to add this feature.
Could you help us to find those places where the errors could happen please?
roggervalf
force-pushed
the
feat-errorListerners
branch
from
79d2caf to
c0a2a55
Compare
|
Interesting that the errorListener is missing, I was also wondering why errors were not appearing, any chance to have a fix for it? |
hi @Visot, I'm still trying to do it in this pr |
packages/core/src/interpreter.ts
Outdated
| event: SingleOrArray<Event<TEvent>> | SCXML.Event<TEvent>, | ||
| payload?: EventData | ||
| payload?: EventData, | ||
| sendError = false |
There was a problem hiding this comment.
Let's remove this; errors should either be unhandled error.* events or "natural" errors thrown from execution of the machine.
There was a problem hiding this comment.
thank you @davidkpiano I just see your message, in my last commit I was replacing this by checking the type in the event, is that ok or should I do in a different way?
|
+1 on this one, super useful if implemented |
packages/core/src/interpreter.ts
Outdated
| reportUnhandledExceptionOnInvocation(errorData, error, id); | ||
| if (this.parent) { | ||
| this.parent.send({ | ||
| type: 'xstate.error', |
There was a problem hiding this comment.
@davidkpiano could you please help me in this case, is it ok this type of error or should it be a different one?
roggervalf
changed the title
eliecerUribe
left a comment
eliecerUribe
left a comment
There was a problem hiding this comment.
Very useful feature for this. Good job! 💯
|
What's left in order to get this PR merged? |
packages/core/src/interpreter.ts
Outdated
| return this.state; | ||
| } | ||
|
|
||
| if (((event as EventObject)?.type || '').includes('error')) { |
There was a problem hiding this comment.
If anything - this should be checked on the _event (and on the appropriate field). It might be "too soon" to check this as the event might not be an EventObject, it actually might be already a SCXML.Event.
This should also check if the type/name of the event starts with error. and not if the type contains an error substring.
also, a nit: optional chaining here and defaulting this to an empty string seems unnecessary:
- the
eventparameter is required - all events should have valid types/names
packages/core/src/interpreter.ts
Outdated
| if (this.parent) { | ||
| this.parent.sendError(event as any); | ||
| } else { | ||
| this.sendError(event as any); |
There was a problem hiding this comment.
This one sounds like it's going to send this event somewhere (like some kind of an actor or something), but it's more like notifying current subscribers.
I also do not (overall) liking how this might make more errors to be swallowed. In general, the error story in XState is currently under-specified so it's somewhat hard to answer "how this should behave now?". I would love to hear out your thoughts about this RFC: statelyai/rfcs#4 . Going through that could also show you my perspective better.
There was a problem hiding this comment.
Looks pretty nice, could you please guide me on the proper changes when you get some chance?
|
I'm sorry that this PR didn't get the attention it deserved in the past. In v5 those parts changed and I think that the issue at hand was already addressed on that branch. |
6 participants
This PR tries to handle this missing feature, where error listeners were not attached.