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

[work in progress] feature/xstate/angular #4638

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ducin
Copy link

@ducin ducin commented Dec 27, 2023

This is a very early implementation that already does its job. It's not ready for a merge yet, however, I'd like to start review with @Andarist asap so that it's easier later on. There might be some setup stuff (or some conventions?) I'm missing, most probably.

usage

export class AppComponent {
  #injector = inject(Injector);
  toggleActor = injectActor(toggleMachine, undefined, {
    injector: this.#injector
  });

  constructor() {
    this.toggleActor.ref.start();
  }

  handleToggleClick() {
    this.toggleActor.send({ type: 'toggle' });
  }
}

Nowadays, this is the standard approach in angular to get dependencies straight into a component (or another service):

  #injector = inject(Injector);
  toggleActor = injectActor(toggleMachine, undefined, {
    injector: this.#injector
  });

injecting the Injector looks odd, but that's actually expected, since there's a whole hierarchy of injectors, so you might get lots of different injectors when needed. And passing the injector via parameter is also a standard approach in libraries (either your call is within, so called, injection context and our adapter would get the injector itself, or one could pass it explicitly via param). All standard.

requirements

As @Andarist mentioned, there has to be a way to allow components having multiple state machines.

angular v17

The adpater exposes a signal-based API (the new Angular standard API for managing synchronous state).

interface InjectedActor<TLogic extends AnyActorLogic> {
  snapshot: Signal<SnapshotFrom<TLogic>>; // signal
  send: ActorRefFrom<TLogic>['send'];
  ref: ActorRefFrom<TLogic>;
}

It resembles react-based adapter's API.

cleanups

the most important part is actually this:
https://github.com/ducin/xstate/blob/feature/xstate/angular/packages/xstate-angular/src/injectActor.ts#L50-L53
all it does is it grabs destroyRef (an angular kinda-equivalent to useEffect return-callback) from a certain injector. Also, pretty standard.

questions

  • wouldn't it make sense for the adapter to automatically start() the machine? Wouldn't it be the most common scenario to immediately start the machine (without the need to do that explicitly)?

further work

I want to finish the following before considering the PR ready:

  • cleanups (e.g. comments), obviously
  • some more examples (working on it)
  • analyzing possibility to provide globally available machines (could be achieved by explicitly creating a service and making it available in certain areas of application, so it's an optional feature)

Copy link

changeset-bot bot commented Dec 27, 2023

⚠️ No Changeset found

Latest commit: 504c296

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@ducin
Copy link
Author

ducin commented Dec 27, 2023

@Andarist what should we do with this?
Angular v17 requires node v18+. Going back to v16 makes little sense for the adapter since there's no stable signal support, and without the signal support the adaption would be significantly smaller (it's the new standard).

error @angular/[email protected]: The engine "node" is incompatible with this module. Expected version "^18.13.0 || >=20.9.0". Got "16.18.1"
error Found incompatible module.

@ducin ducin marked this pull request as draft December 27, 2023 18:07
@Andarist
Copy link
Member

@Andarist what should we do with this?

This might have already been addressed in the meantime. Could you sync with main? Its CI is already using node 20

"@angular/router": "^17.0.0",
"rxjs": "~7.8.0",
"tslib": "^2.3.0",
"xstate": "^5.3.0",
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this dependency list mention @xstate/angular too? It has not been published yet though and examples are outside of the core monorepo... so this might be slightly problematic right now. Maybe it would be worth to split this (before the merge) into 2 different PRs? One could introduce @xstate/angular and the other one could introduce an example using it.

})
export class AppComponent {
@ViewChild('output') outputEl!: ElementRef<HTMLDivElement>;
// protected stateLabel: string = '';
Copy link
Member

Choose a reason for hiding this comment

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

those comments here should be cleaned up before merge

Comment on lines +23 to +25
"paths": {
"@xstate/angular": ["../../packages/xstate-angular"]
}
Copy link
Member

Choose a reason for hiding this comment

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

If we split this PR into two (like mentioned in the other comment) then we could remove this, right? Then @xstate/angular would just get installed from the registry and things would "just work"

Copy link
Member

Choose a reason for hiding this comment

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

It seems that this PR introduces 2 different lockfiles in this example - we should stick to one 😉 I think most of our examples are meant to be using pnpm - but I see that some of them might have some leftover yarn/npm lockfiles too.

Copy link
Member

Choose a reason for hiding this comment

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

I removed the mentioned leftovers here: #4641

Copy link
Member

Choose a reason for hiding this comment

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

this likely shouldnt be touched as part of this PR 😉

Copy link
Member

Choose a reason for hiding this comment

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

I'd probably remove this file altogether. It will just get auto-created in the future when we release new versions of this package.

Copy link
Member

Choose a reason for hiding this comment

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

It would be great to have at least a basic usage example in this README

"angular",
"inject"
],
"author": "Tomasz Ducin <[email protected]>",
Copy link
Member

Choose a reason for hiding this comment

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

All other packages mention David as the author so it feels weird to have a different author here. But I don't know, an "author" field in the MIT-licensed OSS software is a weird concept to me 🤷‍♂️ Maybe we should just remove this field altogether from (almost?) all packages?

"license": "MIT",
"main": "dist/xstate-angular.cjs.js",
"module": "dist/xstate-angular.esm.js",
"type": "module",
Copy link
Member

Choose a reason for hiding this comment

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

is this a requirement? It certainly makes the current exports configuration incorrect since now .js files will be treated as modules but they are actually scripts

Copy link
Member

@Andarist Andarist Dec 28, 2023

Choose a reason for hiding this comment

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

If possible - I would prefer to remove this field from here.

Comment on lines +66 to +70
"peerDependenciesMeta": {
"xstate": {
"optional": true
}
},
Copy link
Member

Choose a reason for hiding this comment

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

If we still have this in other packages we should remove this in another PR

Suggested change
"peerDependenciesMeta": {
"xstate": {
"optional": true
}
},

Comment on lines +58 to +63
"@angular/common": "^17.0.0",
"@angular/compiler": "^17.0.0",
"@angular/compiler-cli": "^17.0.0",
"@angular/core": "^17.0.0",
"@angular/platform-browser": "^17.0.0",
"@angular/platform-browser-dynamic": "^17.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

are all of those required as peer deps? :o it's kinda crazy 😅

Comment on lines +73 to +78
"@angular/common": "^17.0.0",
"@angular/compiler": "^17.0.0",
"@angular/compiler-cli": "^17.0.0",
"@angular/core": "^17.0.0",
"@angular/platform-browser": "^17.0.0",
"@angular/platform-browser-dynamic": "^17.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

do we use anything beyond @angular/core here? I understand that a lot of this might be required in the final app - but are those anyhow related to this standalone~ library? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

If we don't use #is-development for anything within this package then let's remove this (and the associated configuration from the package.json)

Copy link
Member

Choose a reason for hiding this comment

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

What would this potential export do? Is this related to the global actors idea that you have mentioned in the description of this PR?

} as any;

const subscription = actorInstance.subscribe((currentSnapshot) => {
// result.snapshot = snapshot;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// result.snapshot = snapshot;

): InjectedActor<TLogic> {
const injector = injectOptions?.injector ?? inject(Injector);
const destroyRef = injector.get(DestroyRef);
// I'm afraid I stepped into this: https://github.com/angular/angular/issues/34478
Copy link
Member

Choose a reason for hiding this comment

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

While this comment might be relevant to a problem that you have faced it feels out of place here. Did you end up using any of the workarounds mentioned here?

const destroyRef = injector.get(DestroyRef);
// I'm afraid I stepped into this: https://github.com/angular/angular/issues/34478
return runInInjectionContext(injector, () => {
const actorInstance = createActor(logic as any, options).start();
Copy link
Member

Choose a reason for hiding this comment

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

If you sync with main you should be able to remove this cast:

Suggested change
const actorInstance = createActor(logic as any, options).start();
const actorInstance = createActor(logic, options).start();

Comment on lines +29 to +30
options?: ActorOptions<TLogic>,
injectOptions?: injectActorOptions
Copy link
Member

Choose a reason for hiding this comment

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

I'd probably merge those two:

Suggested change
options?: ActorOptions<TLogic>,
injectOptions?: injectActorOptions
{ injector = inject(Injector), ...options }: ActorOptions<TLogic> & { injector?: Injector } = {},

options?: ActorOptions<TLogic>,
injectOptions?: injectActorOptions
): InjectedActor<TLogic> {
const injector = injectOptions?.injector ?? inject(Injector);
Copy link
Member

Choose a reason for hiding this comment

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

is it a common practice to use the default injector like this?

Copy link
Member

Choose a reason for hiding this comment

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

I definitely like that this is an option and that people won't have to always pass injector in explicitly

const destroyRef = injector.get(DestroyRef);
// I'm afraid I stepped into this: https://github.com/angular/angular/issues/34478
return runInInjectionContext(injector, () => {
const actorInstance = createActor(logic as any, options).start();
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't it make sense for the adapter to automatically start() the machine? Wouldn't it be the most common scenario to immediately start the machine (without the need to do that explicitly)?

Yes. That would be best, in my opinion.

I also wonder, what's the story around SSR in Angular? Is it enough to put .start() in OnInit to have this compatible with SSR?

Comment on lines +51 to +52
actorInstance.stop();
subscription.unsubscribe();
Copy link
Member

Choose a reason for hiding this comment

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

I usually find it much less error-prone to dispose of local things first before stopping the external~ resource. It usually leads to removing some edge cases around synchronous notification from within the stopping call or ones related to the fact that the other call might throw and interrupt the whole cleanup. I don't think we have to wrap this in any try/catches or nothing - nothing here should be able to throw (if it does then I'd consider that to be a likely bug in XState). It's just a rule of thumb that I developed over the years and try to follow as much as possible

Suggested change
actorInstance.stop();
subscription.unsubscribe();
subscription.unsubscribe();
actorInstance.stop();

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.

2 participants