-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
First Draft for @xstate/angular #4816
base: main
Are you sure you want to change the base?
Conversation
|
Just saw that the pipeline is failing, and I also did some copy paste mistakes in the package Json, I will fix those tomorrow morning |
"@angular/core": "^17.0.0", | ||
"@testing-library/angular": "15.2.0", | ||
"@testing-library/jest-dom": "^6.1.4", | ||
"ng-packagr": "^17.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How strongly required this is? it might be slightly challenging to incorporate this into our current setup. It's certainly doable but if we wouldn't have to use this... that would be kinda simpler for us
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is required but I will check in the morning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it is needed. Not sure what the best way to move forward is on this one, as using babel/preconstruct doesn't make a whole lot of sense here (from what I can say)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can figure it out on my own before we land this - I don't want to put on you the burden of figuring intricacies of our build pipeline. I wonder, what makes it required? Angular surely has to be able to consume regular npm packages - what's different about a package like this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://angular.io/guide/angular-package-format They are doing pre-optimizations, it would probably work fine.
For now I will just exclude it from preconstruct.
We can also schedule a call and figure out the build together, also feel somewhat bad to put that burden on you :D
I wonder if you had a chance to take a look at #4638 . How would you compare both approaches? |
@Andarist overall the implementation is pretty similar. I am slightly biased here, but I think my implementation is a little more ergonomic and flexible, also I prefer not to expose the user to deal with the injection context. My implementation abstracts this better. Also, this approach provides more flexibility in the way the service is provided, as a service can extend useActor to add additional logic. But as I said, I might be biased here and would appreciate other opinions, maybe @ducin has an opinion on this |
What is the best way forward with this? I'd be more than happy to add tests and schematics if this is of interest (probably in a separate pr though). Also is there anything I can do about the ci/codesandbox issue? |
Tests should be added here - as part of this PR. I'm not sure what do you mean by schematics.
I wouldn't be worried about CodeSandbox. It's not a blocker - but we should bump the node version used by that tool. This can be done here but:
It might be good to schedule that call to talk through the build intricacies if you are still up for it. I would mainly like to just get myself up to speed with the angular ways of doing things ;p |
@Andarist Alrighty I added a couple of tests, let me know what you think. I also checked in with the other Angular GDEs about the ng-packagr. It seems like ng-packagr mostly ensures efficient builds with the angular cli and the guarantee that it works with the bundler. But as Angular Package Format is just a specification it is not a set requirement to use it. So I guess you can try to move forward with preconstruct and I can help test builds if you want. |
I also noticed that the build is now failing, it doesn't really seem to be related to any of my changes unless I am missing something? |
It seems that you have forcefully updated the lockfile or something. I've fixed it with
I suspect it still might be a problem. Could you check what happens with a machine like this? createMachine({
entry: () => { throw new Error('oops') }
}) This error should only be thrown on the client side.
I'll try to do that 👍 |
You were right the error was thrown on the server, but that is now fixed! |
The failing test actually made me aware of a behavior that I did not necessarily expect. |
Could you elaborate on what happens in this scenario?
In every other framework actors are not meant to be started at the server. Each SSR render (yes, ATM machine) is meant to be scoped to the current request. If an actor gets started on the server then it might execute some side-effects and SSR is meant to be pure-ish when it comes to what happens in the components to avoid leaks between requests and stuff like that. Also, we need a "symmetry" - if an actor gets started during SSR... what about the cleanup? |
So the machine will be started if it is injected in a component that is getting rendered. If this is not the case because it is a global state that is not rendered somewhere yet, every state transition will get lost. Here's an example that makes it more tangible
I'd also assume that you easily get hydration mismatch errors if the machine is not started on the server but on the client (not super familiar with SSR, so I might be missing something here) |
So I think what you are saying is that transitions won't get executed on the server side (because the machine is not started yet) and that can lead to a hydration mismatch. I mean, I'm just repeating what you said - I just want to make sure that you are talking about a single problem (I see those 2 as basically the same problem). You are right but I think this is expected. If you want to avoid a hydration mismatch then you need to "stabilize" your initial value that gets SSRed - and not rely on transitions being executed there to achieve that. |
Sorry I could have expressed this better, but I think those are 2 separate issues (at least for me) The other issue is still relevant and unrelated to SSR, but rather to the fact that it is dependent on the component lifecycle. |
@Andarist how do you want to proceed? I can delete/fix the test or adjust the implementation? |
@Injectable({ providedIn: providedIn ?? null }) | ||
class ActorStore implements ActorStoreProps<TLogic> { | ||
public actorRef: Actor<TLogic>; | ||
private _snapshot: WritableSignal<SnapshotFrom<TLogic>>; | ||
public send: Actor<TLogic>['send']; | ||
|
||
public get snapshot() { | ||
return this._snapshot.asReadonly(); | ||
} | ||
|
||
constructor() { | ||
const listener = (nextSnapshot: Snapshot<unknown>) => { | ||
this._snapshot?.set(nextSnapshot as any); | ||
}; | ||
|
||
this.actorRef = useActorRef(actorLogic, options, listener); | ||
this._snapshot = signal(useSelector(this.actorRef as any, (s) => s)()); | ||
this.send = this.actorRef.send; | ||
} | ||
} | ||
return ActorStore; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of creating an injectable like this, I think we can just use an injection token with a factory which should simplify this a lot.
afterNextRender( | ||
() => { | ||
actorRef.start(); | ||
}, | ||
{ phase: AfterRenderPhase.Read } | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we just start it directly? What's the idea of running it after next render?
@niklas-wortmann thanks for this! |
@Andarist playing with this I wonder one thing: So I'm trying to create a Machine that consumes different actors via It works defining the actors inside the machine and using |
any updates? |
This is a first draft for an angular implementation for XState.
As you will notice, it does need some tests, and also for convenience, we could add an angular add schematic, but other than that it is working in the tests I ran.
It did take heavy inspiration from the xstate/vue implementation but also ngrx signalstore.
Let me know what you think!