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.

Sign up for GitHub

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

[v5] Awaitable actors #3977

Closed
wants to merge 4 commits into from
Closed

[v5] Awaitable actors #3977

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d2da099
Make actor thenable
davidkpiano Apr 8, 2023
197eff3
Merge branch 'next' into v5/thenable
davidkpiano Apr 23, 2023
fc34901
Add getOutput + test
davidkpiano Apr 23, 2023
b71269e
Add support for promises, of course
davidkpiano Apr 23, 2023
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
4 changes: 4 additions & 0 deletions packages/core/src/StateMachine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,10 @@ export class StateMachine<
return getPersistedState(state);
}

public getOutput(state: State<TContext, TEvent, TResolvedTypesMeta>) {
return state.output;
}

public createState(
stateConfig:
| State<TContext, TEvent, TResolvedTypesMeta>
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/actors/promise.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ export function fromPromise<T>(
getSnapshot: (state) => state.data,
getStatus: (state) => state,
getPersistedState: (state) => state,
getOutput: (state) => state.data,
restoreState: (state) => state
};

Expand Down
25 changes: 25 additions & 0 deletions packages/core/src/interpreter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,31 @@ export class Interpreter<
return this;
}

public getOutput() {
return this.behavior.getOutput?.(this._state);
}

// thenable
public then(
Copy link
Member

Choose a reason for hiding this comment

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

I don't "feel" this feature. If anything, I'd probably prefer smth like toPromise over this. Sure, we can make things "thenable" - should we though? The actor is not a promise and despite the fact that JS allows us to do this, it feels rather weird.

Since we already have waitFor I also don't necessarily feel the need for toPromise either. It's already simple to do this. Yet a different option would be to just have actor.waitFor.

Our ActorLogic interface starts to be fatter than a turkey on Thanksgiving 😬

Copy link
Member Author

@davidkpiano davidkpiano Jun 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The current problem is there's no easy way to have "wait for actor to complete" for any actor logic, unless you're okay with an optional selector here:

// single argument - wait for actor to be done
const output = await waitFor(someActor);

// current behavior
// PROBLEM: not sure what should be returned
const output = await waitFor(someActor, state => /* ??? */);

What do you think would be a good workaround with waitFor?

Copy link
Member Author

@davidkpiano davidkpiano Aug 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ping @Andarist - need a good solution to this use-case:

// get actor output when the actor is completed
const output = await toPromise(actor);

Can't do this with waitFor().

resolve: (value: SnapshotFrom<TBehavior>) => void,
reject?: (error: any) => void
): PromiseLike<SnapshotFrom<TBehavior>> {
return new Promise((resolvePromise, rejectPromise) => {
if (this.status === ActorStatus.Stopped) {
const output = this.getOutput();
resolvePromise(output);
resolve(output);
return;
}

this.onDone(() => {
const output = this.getOutput();
resolvePromise(output);
resolve(output);
});
});
}

/**
* Starts the interpreter from the initial state
*/
Expand Down
3 changes: 3 additions & 0 deletions packages/core/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1732,6 +1732,8 @@ export interface ActorRef<TEvent extends EventObject, TSnapshot = any>
getSnapshot: () => TSnapshot | undefined;
// TODO: this should return some sort of TPersistedState, not any
getPersistedState?: () => any;
// TODO: this should return some TOutput type
getOutput: () => any;
stop: () => void;
toJSON?: () => any;
// TODO: figure out how to hide this externally as `sendTo(ctx => ctx.actorRef._parent._parent._parent._parent)` shouldn't be allowed
Expand Down Expand Up @@ -1860,6 +1862,7 @@ export interface ActorBehavior<
actorCtx: ActorContext<TEvent, TSnapshot>
) => TInternalState;
getSnapshot?: (state: TInternalState) => TSnapshot;
getOutput?: (state: TInternalState) => any;
getStatus?: (state: TInternalState) => { status: string; data?: any };
start?: (
state: TInternalState,
Expand Down
10 changes: 10 additions & 0 deletions packages/core/test/actor.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,16 @@ describe('spawning promises', () => {

promiseService.start();
});

it('should be awaitable', async () => {
const promiseActor = interpret(
fromPromise(() => Promise.resolve(42))
).start();

const result = await promiseActor;

expect(result).toEqual(42);
});
});

describe('spawning callbacks', () => {
Expand Down
45 changes: 45 additions & 0 deletions packages/core/test/final.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,4 +165,49 @@ describe('final states', () => {
service.send({ type: 'FINISH', value: 1 });
expect(spy).toBeCalledTimes(1);
});

it('should await actors', async () => {
const machine = createMachine({
initial: 'pending',
states: {
pending: {
on: {
RESOLVE: 'done'
}
},
done: {
type: 'final',
data: { count: 42 }
}
}
});

const actor = interpret(machine).start();

setTimeout(() => {
actor.send({ type: 'RESOLVE' });
}, 1);

const data = await actor;

expect(data).toEqual({ count: 42 });
});

it('should await already done actors', async () => {
const machine = createMachine({
initial: 'done',
states: {
done: {
type: 'final',
data: { count: 42 }
}
}
});

const actor = interpret(machine).start();

const data = await actor;

expect(data).toEqual({ count: 42 });
});
});
21 changes: 21 additions & 0 deletions packages/core/test/interpreter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1857,3 +1857,24 @@ it('should throw if an event is received', () => {
)
).toThrow();
});

it('should be awaitable', async () => {
const machine = createMachine({
initial: 'a',
states: {
a: {
after: {
10: 'b'
}
},
b: {
type: 'final',
output: { foo: 'bar' }
}
}
});

const output = await interpret(machine).start();

expect(output).toEqual({ foo: 'bar' });
});