Conversation
|
CodeSee Review Map:Review in an interactive map View more CodeSee Maps Legend |
# Conflicts: # packages/core/src/actor.ts # packages/core/src/behaviors.ts
| this.context._event = | ||
| typeof event.type !== 'string' | ||
| ? (event as LifecycleSignal) | ||
| : toSCXMLEvent(event as TEvent); | ||
|
|
||
| this.current = this.behavior.transition( | ||
| this.current, | ||
| typeof this.context._event.type !== 'string' | ||
| ? (this.context._event as LifecycleSignal) | ||
| : (this.context._event as SCXML.Event<TEvent>).data, | ||
| this.context | ||
| ); |
There was a problem hiding this comment.
This is quite ugly but I didn't want to touch this any further in this PR. Refining the type with TS was not possible here because apparently, TS has problems when it comes to distinguishing strings and symbols:
TS playground
I've just found isSignal helper in the codebase and I could use it here - but OTOH I think that using symbols here is not buying as anything and it's just annoying to work with so I would like to propose adjusting those signals to use plain strings.
| useEffect(() => { | ||
| actorRef.start!(); | ||
| return () => actorRef!.stop!(); | ||
| }); |
There was a problem hiding this comment.
Top-level actorRefs are required to have start method because we need to start them in a "deferred" manner. We can't start them when rendering in the React world - this has to be done in an effect
| start() { | ||
| mailbox.start(); | ||
| }, | ||
| stop() { | ||
| mailbox.clear(); | ||
| }, |
There was a problem hiding this comment.
I think there is an urgent need to define "primitive types" of XState. Quite frankly, I'm getting lost in that - there are a lot of similar, yet slightly different abstractions introduced in the codebase and it's hard to manage their responsibilities because they are not entirely well-defined. I think that I understand most of them but yet the implementation doesn't mirror what I assume and this is confusing.
There was a problem hiding this comment.
Can you list exactly what's confusing?
There was a problem hiding this comment.
Basically what we've discussed last week - atm behaviors are not so pure but they should be. Whatever responsibilities are managed by behaviors should be pulled into actors.
| _event: null as any | ||
| }; | ||
|
|
||
| state = behavior.start ? behavior.start(actorCtx) : state; |
There was a problem hiding this comment.
Same argument as here. We can't execute start when rendering.
| }; | ||
| }, | ||
| start() { | ||
| mailbox.start(); |
There was a problem hiding this comment.
this most likely should also start a behavior or something, but I'm not sure about the relationships between different primitive types (as mentioned in another comment). All tests are passing at the moment - so at the very least this is under-specified and we should make sure those primitive types and communication protocols etc are well-defined and covered by tests.
|
We'll merge this into |
No description provided.