-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(core): errored initial snapshot throws sync w/o observers #5045
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,8 @@ import { | |
createMachine, | ||
fromCallback, | ||
fromPromise, | ||
fromTransition | ||
fromTransition, | ||
toPromise | ||
} from '../src'; | ||
|
||
const cleanups: (() => void)[] = []; | ||
|
@@ -892,4 +893,41 @@ describe('error handling', () => { | |
error_thrown_in_guard_when_transitioning] | ||
`); | ||
}); | ||
|
||
it('error thrown when resolving the initial context should rethrow synchronously', (done) => { | ||
const machine = createMachine({ | ||
context: () => { | ||
throw new Error('oh no'); | ||
} | ||
}); | ||
|
||
const actor = createActor(machine); | ||
|
||
installGlobalOnErrorHandler(() => { | ||
done.fail(); | ||
}); | ||
|
||
expect(() => actor.start()).toThrowErrorMatchingInlineSnapshot(`"oh no"`); | ||
Comment on lines
+897
to
+910
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actors are always async conceptually, a synchronous throw is something that might sound convenient at first but you essentially create 2 error channels by introducing this. Now, if you want to handle errors, u need to both install an error listener and wrap this in try/catch. The guiding principle that we had when designing this was that you should always be notified about errors in the very same way (through error listeners). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Andarist In theory I agree, but in practice, if we know that an actor is going to fail right at the start, rather than later during event processing, it's a much better DX (IMO) to throw right away. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is my feeling too. User experience > theoretical purity There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @boneskull I may have changed my stance on this: https://www.youtube.com/watch?v=JCXZhe6KsxQ&t=294s There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't follow There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @davidkpiano Should I close this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, let's close it for now, given that it's a breaking change. |
||
|
||
setTimeout(() => { | ||
done(); | ||
}, 10); | ||
}); | ||
|
||
it('error thrown when resolving the initial context should reject when wrapped in a Promise', async () => { | ||
const machine = createMachine({ | ||
context: () => { | ||
throw new Error('oh no'); | ||
} | ||
}); | ||
|
||
const actor = createActor(machine); | ||
|
||
try { | ||
await toPromise(actor.start()); | ||
fail(); | ||
} catch (err) { | ||
expect((err as Error).message).toEqual('oh no'); | ||
} | ||
}); | ||
Comment on lines
+917
to
+932
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't work like you think it does. This error is still very much thrown synchronously. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what you mean. If There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIRC, It's the |
||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we can do this a different way without creating a temp sub
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidkpiano The folded section in this comment has an alternate proposal, which I explored but didn't get working fully.