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

Microsteps #1045

Merged
merged 57 commits into from
Apr 3, 2020
Merged

Microsteps #1045

merged 57 commits into from
Apr 3, 2020

Conversation

davidkpiano
Copy link
Member

@davidkpiano davidkpiano commented Mar 4, 2020

This PR adds first-class support for representing microsteps through:

  • machine.microstep(state, event)
  • service.onMicrostep(state => ...) (Will not be supported quite yet)

@changeset-bot
Copy link

changeset-bot bot commented Mar 4, 2020

🦋 Changeset is good to go

Latest commit: 7f3b848

We got this.

This PR includes changesets to release 10 packages
Name Type
xstate Major
@xstate/analytics Major
@xstate/graph Major
@xstate/immer Major
@xstate/react Major
@xstate/scxml Patch
@xstate/test Major
@xstate/vue Major
@xstate/viz Patch
@xstate/viz-example Patch

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 master to next March 4, 2020 18:53
@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 4, 2020

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 7f3b848:

Sandbox Source
nervous-kapitsa-46cvd Configuration
vigorous-hawking-mopkx Configuration

@davidkpiano davidkpiano marked this pull request as ready for review March 15, 2020 20:59
@davidkpiano davidkpiano changed the title Microsteps (WIP) Microsteps Mar 15, 2020
Copy link
Member

@Andarist Andarist left a comment

Choose a reason for hiding this comment

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

This is definitely an improvement 👍

But... if we want to adhere to SCXML semantics I find it hard to keep some of the things, such as resolving some actions (like assign) in the machine rather than in the interpreter.

Take a look at Evaluation of Executable Content - we might notice that actions should actually be grouped in "blocks" where if an exception happens during block's execution the rest of the actions in that block don't happen (only from that very block! other blocks proceed with executions as usual) and an error event gets raised in such a situation. With some things being resolved in the machine it's impossible (or at least hard and not worth it) to match those semantics - because if we have a block such as [send(FOO, { to: 'invalid' }), assign({ answer: 42 })] then this assign action should actually never happen and the context should not get updated.

IMHO we should explore a direction of Machine being pure and very minimal - receiving a current state, an event and returning a description of what should happen during a single transition (in a microstep sense), but what should happen ought to be resolved by the caller as ultimately it is the stateful entity. This would clearly draw a line between the responsibilities of each.

packages/core/test/json.test.ts Outdated Show resolved Hide resolved
packages/core/test/scxml.test.ts Outdated Show resolved Hide resolved
packages/core/test/transient.test.ts Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
packages/core/test/activities.test.ts Show resolved Hide resolved
packages/core/src/stateUtils.ts Outdated Show resolved Hide resolved
packages/core/src/stateUtils.ts Show resolved Hide resolved
packages/core/src/stateUtils.ts Outdated Show resolved Hide resolved
packages/core/src/stateUtils.ts Show resolved Hide resolved
packages/core/src/stateUtils.ts Show resolved Hide resolved
@Andarist
Copy link
Member

It would be great to add changesets describing public-facing changes & additions, so we don't forget about them in the future. Could you also add tests for your latest changes?

}
```

- Assign actions (via `assign()`) will now be executed "in order", rather than automatically prioritized. They will be evaluated after previously defined actions are evaluated, and actions that read from `context` will have those intermediate values applied, rather than the final resolved value of all `assign()` actions taken, which was the previous behavior.
Copy link
Member

Choose a reason for hiding this comment

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

This probably requires removing information about assign being prioritized from the docs.

@davidkpiano davidkpiano merged commit f86f151 into next Apr 3, 2020
@davidkpiano davidkpiano deleted the v5/microstep branch April 3, 2020 12:17
@github-actions github-actions bot mentioned this pull request Apr 3, 2020
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