[v5] Remove batching #2869
[v5] Remove batching #2869
Conversation
🦋 Changeset detectedLatest commit: 0af90a3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
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 |
CodeSee Review Map:Review in an interactive map View more CodeSee Maps Legend |
| const service = interpret(machine).start(); | ||
|
|
||
| expect(service.state.context.childOrigin).toBe('callback_child'); | ||
| interpret(machine) |
There was a problem hiding this comment.
q: did anything change here that this test has to be asynchronous right now?
There was a problem hiding this comment.
Nope, just a refactoring to match how other services are "idiomatically" tested.
Although, we should discourage reading service.state directly, and maybe disallow it in favor of service.getSnapshot()
There was a problem hiding this comment.
Nope, just a refactoring to match how other services are "idiomatically" tested.
I agree that this might be "idiomatically" correct, however, I've found out that writing "non-idiomatic" tests (aka tests executed synchronously) can reveal more edge cases while writing those tests. It's because some problems only manifest themselves when we deal with synchronous execution, ping-ponging between different actors - this is true for XState, Redux-Saga, and even RxJS because those problems can be classified as scheduling problems and all of those libs deal with this and try to manage the complexity involved with communicating different "processes" etc
Co-authored-by: Pat Cavit <github@patcavit.com>
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
| @@ -240,32 +240,42 @@ export class StateMachine< | |||
| return transitionNode(this.root, state.value, state, _event) || []; | |||
| } | |||
|
|
|||
| public get first(): State<TContext, TEvent> { | |||
| const pseudoinitial = this.resolveState( | |||
| private get preInitialState(): State<TContext, TEvent> { | |||
There was a problem hiding this comment.
q: how do we define a "pre initial state"? could you add a comment above this method that would explain this?
There was a problem hiding this comment.
Added a comment. This is the initial state before evaluating any microsteps. The preInitialState may differ from the initialState e.g. if the preInitialState has an eventless transition to a different state.
# Conflicts: # packages/core/src/actor.ts # packages/core/src/behaviors.ts
packages/core/src/Mailbox.ts
Outdated
| if (this.index > this.events.length - 1) { | ||
| this.events.length = 0; | ||
| this.index = 0; | ||
| } |
There was a problem hiding this comment.
This strategy doesn't scale because we retain all the queued items up until we drain the queue. The items that were already processed should be immediately disposed because we don't have any use for them from that point in time.
I've reimplemented this mailbox using a simple linked list to avoid this problem altogether:
37547c3
packages/core/src/interpreter.ts
Outdated
| return this; | ||
| } | ||
|
|
||
| private flush() { | ||
| this.mailbox.status = 'processing'; | ||
| let event = this.mailbox.dequeue(); |
There was a problem hiding this comment.
This implementation doesn't scale too well with multiple consumers of the mailbox because a lot of the delicate queueing/dequeuing management becomes the responsibility of each consumer. I think I've managed to avoid this in this PR:
37547c3
With these changes, the consumer only needs to provide a "process" callback to the created mailbox and call simple start, clear and enqueue methods. Thanks to that I've been able to reuse easily this Mailbox implementation in other 2 places that were managing this:
00f1d74
e530b5b
Andarist/mailbox unification
This PR removes batching and (somewhat related) the scheduler - refactored to be a simple mailbox.