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] Allow guard objects to reference other guard objects #4064

Merged
merged 9 commits into from
Jun 17, 2023

Conversation

davidkpiano
Copy link
Member

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Jun 8, 2023

🦋 Changeset detected

Latest commit: 100932e

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

This PR includes changesets to release 1 package
Name Type
xstate 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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 8, 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 100932e:

Sandbox Source
XState Example Template Configuration
XState React Template Configuration

@ghost
Copy link

ghost commented Jun 8, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

): GuardDefinition<TContext, TEvent> {
// TODO: do this recursively and check for cycles
if (isString(guardConfig)) {
Copy link
Member

Choose a reason for hiding this comment

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

All of this feels a tad complicated - but it's not exactly a unique problem to this function (similar things happen when "resolving" actions).

I think that the main mistake that we do here is that we try to resolve (through toGuardDefinition) too eagerly. We always need to have this resolution~ step when finally checking guards since we need to be inside the execution context of a machine. It's kinda pointless to resolve this twice.

Let's roll with this for now but I think that this should be flagged as something to refactor.

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.

Don't forget about a changeset! 😉

expect(actorRef.getSnapshot().matches('b')).toBeTruthy();
});

it('should be possible to reference a composite guard that references other guards recursively', () => {
Copy link
Member

Choose a reason for hiding this comment

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

@davidkpiano you noted down in a code comment that we should handle things recursively but I pushed out some new tests here and this things seems to work. Am I missing some cases that you were thinking about?

Copy link
Member Author

Choose a reason for hiding this comment

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

This still only goes one level deep; try something like:

guards: {
  ref1: { type: 'ref2' },
  ref2: { type: 'ref3' },
  ref3: () => true
}

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
@davidkpiano davidkpiano merged commit 0478972 into next Jun 17, 2023
@davidkpiano davidkpiano deleted the v5/even-higher-level-guards branch June 17, 2023 03:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants