Skip to content
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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions packages/core/src/createActor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,26 @@ export class Actor<TLogic extends AnyActorLogic>
// TODO: rethink cleanup of observers, mailbox, etc
return this;
case 'error':
// in this case, creation of the initial snapshot caused an error.

// **if the actor has no observer when start() is called**, this error would otherwise be
// thrown to the global error handler. to prevent this--and allow start() to be wrapped in try/catch--we
// create a temporary observer that receives the error (via _reportError()) and rethrows it from this call stack.
Comment on lines +496 to +498
Copy link
Member

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

Copy link
Contributor Author

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.

let err: unknown;
let errorTrapSub: Subscription | undefined;
if (!this.observers.size) {
errorTrapSub = this.subscribe({
error: (error) => {
// we cannot throw here because it would be caught elsewhere and rethrown as unhandled
err = error;
}
});
}
this._error((this._snapshot as any).error);
errorTrapSub?.unsubscribe();
if (err) {
throw err;
}
return this;
}

Expand Down
40 changes: 39 additions & 1 deletion packages/core/test/errors.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ import {
createMachine,
fromCallback,
fromPromise,
fromTransition
fromTransition,
toPromise
} from '../src';

const cleanups: (() => void)[] = [];
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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).

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my feeling too. User experience > theoretical purity

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidkpiano Should I close this?

Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The 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. toPromise isn't ever called during this test case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean. If createActor threw synchronously, wouldn't this test fail?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, It's the actor.start that throws, before its return value can even get passed to toPromise

});