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] System (receptionist) #3837

Merged
merged 33 commits into from
Mar 29, 2023
Merged

[v5] System (receptionist) #3837

merged 33 commits into from
Mar 29, 2023

Conversation

davidkpiano
Copy link
Member

This PR creates an implicit system and provides it in the machine implementations:

const machine = createMachine({
  id: 'parent',
  initial: 'a',
  states: {
    a: {
      invoke: [
        {
          src: fromCallback((_, receive) => {
            receive((event) => {
              expect(event.type).toBe('HELLO');
            });
          }),
          key: 'receiver'
        },
        {
          src: createMachine({
            id: 'childmachine',
            entry: (_ctx, _ev, { system }) => {
              const receiver = system.get('receiver');

              if (receiver) {
                receiver.send({ type: 'HELLO' });
              }
            }
          })
        }
      ]
    }
  }
});

@changeset-bot
Copy link

changeset-bot bot commented Feb 15, 2023

🦋 Changeset detected

Latest commit: 84af883

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

This PR includes changesets to release 1 package
Name Type
xstate 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 base branch from main to next February 15, 2023 16:48
@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 15, 2023

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 84af883:

Sandbox Source
XState Example Template Configuration
XState React Template Configuration

@ghost
Copy link

ghost commented Feb 15, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@tivac
Copy link
Contributor

tivac commented Feb 19, 2023

(Sorry if this is an obvious question, can't look through the changes atm)

Is the system accessible from the interpreter for outside use as well?

@with-heart
Copy link
Contributor

Love this so much. It's a testament to the system yall have built that such a powerful feature requires so little to code implement. Super excited for this!

Co-authored-by: with-heart <with.heart+git@pm.me>
@davidkpiano
Copy link
Member Author

(Sorry if this is an obvious question, can't look through the changes atm)

Is the system accessible from the interpreter for outside use as well?

It is "not" (at least not naturally)... but of course, once you have access to the { system } grabbed from an action, we can't stop you from passing that reference around 🤷

@tivac
Copy link
Contributor

tivac commented Feb 20, 2023

Is there a previous discussion on why that isn't allowed?

With how we use child machines being able to send events to specific ones instead of blasting everybody or manually plumbing through the parent seems really valuable.

@davidkpiano
Copy link
Member Author

Is there a previous discussion on why that isn't allowed?

With how we use child machines being able to send events to specific ones instead of blasting everybody or manually plumbing through the parent seems really valuable.

Maybe I'm misreading the question, but from any descendant machine within the same system, you have access to the system, so yes, you can send events to specific actors ('blah' in this case) instead of "blasting".

CleanShot 2023-02-20 at 14 25 14@2x

@tivac
Copy link
Contributor

tivac commented Feb 20, 2023

@davidkpiano I'm talking about something like this:

import { createMachine, interpret } from 'xstate';

const rootMachine = createMachine({
  /* ... */
});

const service = interpret(rootMachine);

service.start();

const receiver = service.system.get("receiver");

if(receiver) {
  receiver.send({ type: 'HELLO' });
}

Since we're using xstate-component-tree to manage components that are being rendered, and sometimes components want to fire an event back into the statechart that spawned them. Currently xstate-component-tree offers a .broadcast(<event>) API that literally sends the event into every running statechart but that's a little coarse and dangerous for my tastes.

I guess since xstate-component-tree is actively tracking the running children at any given time it could provide a service-style API, maybe, but I was hoping that this would let me rip all that logic out of my code and use something built-in and more standard.

@davidkpiano
Copy link
Member Author

@tivac Ah okay, I understand; you want to access the service completely outside the machine.

I agree that this should be made possible by the API you've described:

const receiver = service.system.get('receiver');

I'll add tests for this.

packages/core/src/types.ts Outdated Show resolved Hide resolved

if (key !== undefined) {
keyedActors.delete(key);
reverseKeyedActors.delete(actorRef);
Copy link
Member

Choose a reason for hiding this comment

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

this one could be called unconditionally, otoh - it kinda doesn't matter cause you are using WeakMap, in a way... there is no point in this .delete call at all, the engine itself should just clean this up - that's the whole point of the Weak part in the WeakMap

davidkpiano and others added 8 commits March 28, 2023 12:28
…the system (#3927)

* Fixed memory leak by separating sessionId from actor registration in the system

* Throw error if actor with system ID already exists

---------

Co-authored-by: David Khourshid <davidkpiano@gmail.com>
export interface ActorContext<
TEvent extends EventObject,
TSnapshot,
TSystem extends ActorSystem<any> = ActorSystem<any>
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this shouldn't accept the same generic as ActorSystem itself accepts. Not a big deal though and I don't have a strong opinion about this right now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can change it later; if for whatever reason a dev needs to manually construct this, this type can be typeof someSystem rather than ActorSystemInfoFrom<typeof someSystem>

Comment on lines +1616 to +1618
/**
* The system ID to register this actor under
*/
Copy link
Member

Choose a reason for hiding this comment

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

#yoda

Comment on lines +2007 to +2013
export interface ActorSystem<T extends ActorSystemInfo> {
_bookId: () => string;
_register: (sessionId: string, actorRef: AnyActorRef) => string;
_unregister: (actorRef: AnyActorRef) => void;
_set: <K extends keyof T['actors']>(key: K, actorRef: T['actors'][K]) => void;
get: <K extends keyof T['actors']>(key: K) => T['actors'][K] | undefined;
}
Copy link
Member

Choose a reason for hiding this comment

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

Don't worry about this right now but I find defining a type like this here extremely annoying to work with. It's not exactly an "abstract" type that comes with no direct implementation. It has a single implementation in the system.ts and whenever I cmd+click on ActorSystem I'm being brought to here by the IDE which isn't that helpful (going to the implementation in the system.ts would be way more useful in this instance)

Copy link
Member Author

Choose a reason for hiding this comment

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

Want a class instead?

Copy link
Member

Choose a reason for hiding this comment

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

the current runtime code is fine - but the type definition could be derived from the implementation or just declared next to the implementation then jumping to it would at least bring us to the correct file

Copy link
Member Author

Choose a reason for hiding this comment

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

the current runtime code is fine - but the type definition could be derived from the implementation or just declared next to the implementation then jumping to it would at least bring us to the correct file

I'm open to having this be refactored, if you want to (not immediately sure how to do this)

@davidkpiano davidkpiano merged commit 6155360 into next Mar 29, 2023
@davidkpiano davidkpiano deleted the v5/system-1 branch March 29, 2023 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants