Skip to content

V5: History refactor#987

Merged
davidkpiano merged 44 commits intonextfrom
v5/history
Feb 22, 2020
Merged

V5: History refactor#987
davidkpiano merged 44 commits intonextfrom
v5/history

Conversation

@davidkpiano
Copy link
Member

This PR refactors the state history resolution algorithm to match the SCXML algorithm.

  • state.historyValue eliminated, replaced with state.historyMap
  • A lot of code removed! 🎉

@davidkpiano davidkpiano requested a review from Andarist February 9, 2020 06:57
@changeset-bot
Copy link

changeset-bot bot commented Feb 9, 2020

🦋 Changeset is good to go

Latest commit: 0e24ea6

We got this.

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 February 9, 2020 06:57
@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 9, 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.

Copy link
Collaborator

@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.

Overall it's a very good simplification 🎉 I've left some comments and few questions (or things that I'm unsure about). Let's discuss them and at least create an action points list (as issues?) - we don't necessarily have to hold off this PR until all of them get resolved.

I've also liked how you have added a few PR comments after creating this PR which helped me a little bit to understand moved parts quicker 👍

Copy link
Collaborator

@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.

I have left some few minor comments, I think we can address most of them as part of this PR - but if something is too big of a change, then just go ahead and merge this in. We can always iterate over this.

I feel confident that this is a good refactor 👍 and the fact that the implementation got closer to most of the SCXML algorithms is a huge deal. It should be way easier to conform to their semantics when using their algorithms whenever possible.

@Andarist
Copy link
Collaborator

Oh - I forgot, it would be great to describe in short every user-facing changes in changesets.

@davidkpiano davidkpiano merged commit cfe9bb2 into next Feb 22, 2020
@davidkpiano davidkpiano deleted the v5/history branch February 22, 2020 16:07
// ...
green: {
on: {
NOTHING: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

so this only breaks reentering using internal transitions? the description put upfront is quite abstract, would be good to mention concrete scenarios when this thing matters

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll clarify this in the docs, but an internal self-transition in a leaf node is better by not giving a target at all (which is how SCXML represents it) anyway

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.

3 participants