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

V5: MachineNode (and StateNode refactor) #953

Merged
merged 51 commits into from
Feb 5, 2020
Merged

Conversation

davidkpiano
Copy link
Member

@davidkpiano davidkpiano commented Jan 19, 2020

This PR splits StateNode into MachineNode and StateNode:

  • MachineNode extends StateNode
  • All machine-specific methods now only belong to MachineNode
  • Functionality/logic for nodes is separated into functions in nodeUtils.ts
  • context is removed as a 3rd parameter from new MachineNode(config, options, context).

This is an intermediate (refactor) PR, and will be the basis for future PRs.

Addresses #655

@changeset-bot
Copy link

changeset-bot bot commented Jan 19, 2020

🦋 Changeset is good to go

Latest commit: 1cf4186

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 January 19, 2020 20:50
@Andarist Andarist changed the base branch from next to master January 19, 2020 22:00
@Andarist Andarist changed the base branch from master to next January 19, 2020 22:00
@davidkpiano davidkpiano requested review from Andarist and removed request for mogsie January 19, 2020 23:40
commit 94f9b6e
Merge: b497e60 acf095a
Author: David Khourshid <davidkpiano@gmail.com>
Date:   Sun Jan 19 15:51:40 2020 -0500

    Merge branch 'master' into next

commit b497e60
Merge: b122406 e0801cb
Author: David Khourshid <davidkpiano@gmail.com>
Date:   Sun Jan 19 15:50:31 2020 -0500

    Merge branch 'next' of ssh://github.com/davidkpiano/xstate into next

commit acf095a
Merge: 2a3fea1 7367de2
Author: David Khourshid <davidkpiano@gmail.com>
Date:   Sun Jan 19 12:03:53 2020 -0500

    Allow @xstate/fsm to accept actions bag (#946)

    Allow @xstate/fsm to accept actions bag

commit 7367de2
Author: Mateusz Burzyński <mateuszburzynski@gmail.com>
Date:   Sun Jan 19 09:51:53 2020 +0100

    Add changeset

commit ec4f089
Author: Mateusz Burzyński <mateuszburzynski@gmail.com>
Date:   Sun Jan 19 00:05:51 2020 +0100

    Add tests for fsm action bag

commit 6847f49
Author: Mateusz Burzyński <mateuszburzynski@gmail.com>
Date:   Sat Jan 18 13:49:31 2020 +0100

    Update @xstate/react/fsm docs

commit 7a6208b
Author: Mateusz Burzyński <mateuszburzynski@gmail.com>
Date:   Sat Jan 18 13:46:16 2020 +0100

    Tweak implementation of action bag updating

commit f7b173e
Author: Mateusz Burzyński <mateuszburzynski@gmail.com>
Date:   Sat Jan 18 13:37:27 2020 +0100

    Add docs for options object for fsm's `createMachine`

commit fc49a43
Author: Mateusz Burzyński <mateuszburzynski@gmail.com>
Date:   Sat Jan 18 09:49:44 2020 +0100

    Add the same constraint to TContext type everywhere

commit 0d109fc
Author: Mateusz Burzyński <mateuszburzynski@gmail.com>
Date:   Fri Jan 17 07:46:22 2020 +0100

    Move action bag support to createMachine

commit 76068d2
Author: Mateusz Burzyński <mateuszburzynski@gmail.com>
Date:   Wed Jan 15 23:27:26 2020 +0100

    Allow @xstate/fsm to accept actions bag

commit 2a3fea1
Author: David Khourshid <davidkpiano@gmail.com>
Date:   Sat Jan 18 17:11:38 2020 -0500

    Allow any event to be raised (#952)

    * Allow any event to be raised. Fixes #949

    * Add changeset

    * Simplify tests

commit e0801cb
Merge: f34d149 b602e0c
Author: David Khourshid <davidkpiano@gmail.com>
Date:   Sun Jan 5 23:52:28 2020 -0500

    Merge branch 'master' into next
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 31, 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 1cf4186:

Sandbox Source
strange-bardeen-ofnwp Configuration
hidden-shape-gygfs Configuration

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.

I've only done a preliminary review - still have to go through the biggest files 😃 Should do that later today.

docs/guides/ids.md Show resolved Hide resolved
packages/core/src/Machine.ts Show resolved Hide resolved
packages/core/src/State.ts Show resolved Hide resolved
packages/core/src/types.ts Show resolved Hide resolved
packages/core/test/machine.test.ts Show resolved Hide resolved
packages/core/test/machine.test.ts Show resolved Hide resolved
guards,
services,
delays,
context = this.context
Copy link
Member

Choose a reason for hiding this comment

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

I believe this has a slight bug - we can have a machine config with a defined context and create a machine based on it with context option. If we call withConfig on it later we might end up with an incorrect value of context here because it will be the one from this.options.context, but it should actually always just point to this.context (which is a resolved context).

I would just recommend removing this line from here and use this.context directly when calling MachineNode constructor those few lines below.

Copy link
Member

Choose a reason for hiding this comment

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

If my hunch was right about this being a slight bug then it would be good to add a test covering this.

Copy link
Member Author

Choose a reason for hiding this comment

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

The line was removed (this code is outdated) - does this still apply?

Copy link
Member

Choose a reason for hiding this comment

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

I think the code before the change had this slight bug, whereas the new version doesnt have it - so it has fixed the issue, but can potentially regress in the future if not covered by a test.

@@ -0,0 +1,15 @@
---
Copy link
Member

Choose a reason for hiding this comment

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

This doesnt matter much here, but I believe those should get their own changesets. Think about those as separate entries - I think making them separate will also generate slightly different changelog. This now is a single multi-line entry, whereas they could be punctuated (if you would use separate changesets). Using separate changesets has also other benefits - including more flexibility because separate changesets can have different "types" (major/minor/patch) and when changelog is generated those "types" are grouped together.

"Prerelease" changesets are kept when releasing prerelease versions (because when exiting the prerelease stage all of them are used to generate a single "final" changelog entry), so keeping them separate will make reviewing them easier when exiting the prerelease stage (we might want to remove no longer relevant stuff there, so they dont get listed in the final changelog entry).

@davidkpiano davidkpiano merged commit 7e7da80 into next Feb 5, 2020
@davidkpiano davidkpiano deleted the v5/node-refactor branch February 5, 2020 22:59
@github-actions github-actions bot mentioned this pull request Feb 5, 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.

None yet

3 participants