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

Replace getters on MachineSnapshot with methods #4467

Merged
merged 5 commits into from
Nov 21, 2023

Conversation

Andarist
Copy link
Member

closes #2498

Copy link

changeset-bot bot commented Nov 16, 2023

🦋 Changeset detected

Latest commit: 0d0a0f9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
xstate Minor

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

Copy link

codesandbox-ci bot commented Nov 16, 2023

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 0d0a0f9:

Sandbox Source
XState Example Template Configuration
XState React Template Configuration

Copy link
Member

@davidkpiano davidkpiano left a comment

Choose a reason for hiding this comment

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

Actually we need to change nextEvents - going to take the opportunity to do it here

Copy link
Member

@davidkpiano davidkpiano left a comment

Choose a reason for hiding this comment

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

JK this is fine

/**
* The next events that will cause a transition from the current state.
*/
getNextEvents: (
Copy link
Member Author

Choose a reason for hiding this comment

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

One thing that I'm not that fond of here is the name of this. getNextEvents name doesn't match what is returned from this method. What other options do we have on the table? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I was going to get rid of this and have a getNextTransitions function instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

What it would return?

Copy link
Member

Choose a reason for hiding this comment

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

Flat map of own transitions of all state nodes

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'm not sure if that would be that helpful in practice. Or at least that it opens up the return value in a way that we perhaps don't have to. I know I'm a broken record but IMHO such a public piece of the API shouldn't expose things such as guards and actions to people. For most purposes those are really implementation details and we should steer people towards solving their needs in different ways. I don't want to neglect the introspection needs but that can be solved with a dedicated API - the user would have to do the introspection more explicitly.

But even putting those concerns aside, I think that a runtime API like this should at least account for forbidden transitions. This isn't currently solved by the nextEvents API either so there is that but I think that it's sort of a footgun. If the primary use case of this is to generate the UI dynamically based on this then we should account for this because it's so easy to forget about it. Even transition shadowing is a problem here (a: { on: { FOO: {} }, states: { a1: { on: { FOO: {} } } } }). If we don't solve problems like this then what's even the purpose of this API? I'd like to think that the users know how to flat-map things and that this alone isn't a strong reason to introduce an API. Especially that the audience for this is limited - most users certainly don't care about this.

All in all - providing a flat list of transitions/event descriptors isn't solving a meaningful problem, I think. Hierarchy is baked into XState and IMHO APIs like this should account for it. The introspection API wouldn't solve this completely either but I think that with an API like this, it's way more apparent to the user that they are interacting with something special and that they might have to stitch more things manually to achieve what they want. The introspection API might intentionally be quite "raw".

davidkpiano and others added 3 commits November 20, 2023 07:42
* state.configuration -> state.nodes

* Changeset

* Rename Machine -> createMachine and .nodes -> ._nodes

* Rename Machine -> createMachine and .nodes -> ._nodes
@Andarist Andarist merged commit 3c71e53 into next Nov 21, 2023
2 checks passed
@Andarist Andarist deleted the remove-getters-from-machine-snapshot branch November 21, 2023 12:25
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.

Remove Object.defineProperty usage in v5
2 participants