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

[v5] Explicit spawn #3148

Merged
merged 98 commits into from
May 26, 2022
Merged

[v5] Explicit spawn #3148

merged 98 commits into from
May 26, 2022

Conversation

davidkpiano
Copy link
Member

@davidkpiano davidkpiano commented Mar 12, 2022

This PR makes spawn more explicit and introduces the following changes:

  • The external (magical) spawn function and related CapturedState code has been removed
  • Spawning in initial context now comes from an object passed into initial context:
-import { spawn } from 'xstate';

// ...

-context: () => ({
+context: ({ spawn }) => ({
  ref: spawn(someBehavior)
}),
  • Spawning in assign actions now comes from the 3rd "meta" argument:
-import { spawn } from 'xstate';

// ...

actions: assign({
- ref: (ctx, e) => {
+ ref: (ctx, e, { spawn }) => {
    return spawn(someBehavior);
  }
})
  • Spawning actors from string references is now possible:
actions: assign({
  ref: (ctx, e, { spawn }) => {
    return spawn('someBehavior');
  }
}),

// ...

{
  actors: {
    someBehavior: (ctx, e) => /* return a Behavior */
  }
}
  • Event observables work as before, sending events to the parent:
invoke: {
  src: () => fromEventObservable(() => value$.pipe(map(val => ({ type: 'VAL', val })))),
},
on: {
  VAL: {
    actions: (ctx, e) => { console.log(e.val) }
  }
}
  • Observables with fromObservable deliver values rather than events:
 invoke: {
-  src: () => fromObservable(() => value$.pipe(map(val => ({ type: 'VAL', val })))),
+  src: () => fromObservable(() => value$),
+  onEmit: {
+    actions: (ctx, e) => { console.log(e.data) }
+  }
 },
-on: {
-  VAL: {
-    actions: (ctx, e) => { console.log(e.val) }
-  }
}

This PR closes STA-1269

@changeset-bot
Copy link

changeset-bot bot commented Mar 12, 2022

🦋 Changeset detected

Latest commit: 7e2c649

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
xstate Major
@xstate/vue Major
@xstate/react Major
@xstate/graph Major
@xstate/immer Major
@xstate/inspect Major
@xstate/svelte Major
@xstate/test Major

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

@davidkpiano davidkpiano changed the title V5/improved spawn 1 [v5] Spawn refactor Mar 12, 2022
@davidkpiano davidkpiano mentioned this pull request Mar 12, 2022
@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 12, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 7e2c649:

Sandbox Source
XState Example Template Configuration
XState React Template Configuration

@davidkpiano davidkpiano changed the title [v5] Spawn refactor [v5] Explicit spawn Mar 12, 2022
@ghost
Copy link

ghost commented Mar 12, 2022

👇 Click on the image for a new way to code review
  • Make big changes easier — review code in small groups of related files

  • Know where to start — see the whole change at a glance

  • Take a code tour — explore the change with an interactive tour

  • Make comments and review — all fully sync’ed with github

    Try it now!

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

@davidkpiano davidkpiano marked this pull request as ready for review March 13, 2022 14:01
@christoph-fricke
Copy link

  • Spawning actors from string references is now possible:
actions: assign({
  ref: (ctx, e, { spawn }) => {
    return spawn('someBehavior');
  }
}),

// ...

{
  actors: {
    someBehavior: (ctx, e) => /* return a Behavior */
  }
}

Is there a reason for naming the config object actors instead of behaviors? It only contains behaviors that are used to spawn actors, so I would expect it to be named behaviors. Storing behaviors under an actor key might lead to confusion around behaviors and actors?!

@davidkpiano
Copy link
Member Author

  • Spawning actors from string references is now possible:
actions: assign({
  ref: (ctx, e, { spawn }) => {
    return spawn('someBehavior');
  }
}),

// ...

{
  actors: {
    someBehavior: (ctx, e) => /* return a Behavior */
  }
}

Is there a reason for naming the config object actors instead of behaviors? It only contains behaviors that are used to spawn actors, so I would expect it to be named behaviors. Storing behaviors under an actor key might lead to confusion around behaviors and actors?!

Good feedback; open to renaming this to behaviors

@davidkpiano
Copy link
Member Author

@Andarist I reworked the actors to do the original thing - send events directly to the parent. It turns out that this was a better approach that avoided the complexity of dealing with observers and implicit observer method calls.

This is also inspired by a relevant approach in Akka with dealing with futures (promises): https://blog.rockthejvm.com/pipe-pattern/

I think you'll appreciate this change 👍

Comment on lines +332 to +333
case nextEventType:
return event.data;
Copy link
Member

Choose a reason for hiding this comment

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

This is an unused code. It should either be removed or this event should be sent from here:

self._parent?.send(toSCXMLEvent(value, { origin: self }));

and the self._parent.send(...) should be performed here

origin: self
})
);
return event.data;
Copy link
Member

@Andarist Andarist May 18, 2022

Choose a reason for hiding this comment

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

We are expected to return T from here but this event.data is basically any/unknown because it's a rejection value. We should return undefined here:

Suggested change
return event.data;
return undefined;

(and add a test for this)

Copy link
Member Author

Choose a reason for hiding this comment

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

Curious on your thoughts here: since we want to represent an accurate promise snapshot, what do you think about representing promise snapshots as an "Either type" like other languages have?

Copy link
Member

Choose a reason for hiding this comment

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

So based on T we'd type the actual snapshot as { ok: true, value: T } | { ok: false, error: unknown } (or smth like that)?

self.send({ type: completeEventType });
}
});
return state;
Copy link
Member

Choose a reason for hiding this comment

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

return state is technically correct within this whole behavior but in practice, this is return undefined. I wonder if it's worth to change that to make this more visible - right now a reader could assume that some state can be associated with this behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I rename state to snapshot to make this more clear?

Copy link
Member

Choose a reason for hiding this comment

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

That could be done (i dont mind using state here though). The main point was that this particular behavior wont have non-undefined state/snapshot and when reading through those return statements it looks like it can

…undant code (#3319)

* Fixed `ObservableActorRef` subscriptions and remove some outdated/redundant code

* Add changesets
@Andarist Andarist merged commit 7a68cbb into next May 26, 2022
@Andarist Andarist deleted the v5/improved-spawn-1 branch May 26, 2022 10:09
@huan
Copy link
Contributor

huan commented May 26, 2022

Congratulations!

Could we expect the v5 to be published soon to the xstate@next NPM? :-D

@Andarist
Copy link
Member

It's already available as @alpha

@huan
Copy link
Contributor

huan commented May 26, 2022

Awesome, thank you very much for publishing it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants