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

[core] Add state.can(event) #2546

Merged
merged 14 commits into from
Sep 10, 2021
Merged

[core] Add state.can(event) #2546

merged 14 commits into from
Sep 10, 2021

Conversation

davidkpiano
Copy link
Member

@davidkpiano davidkpiano commented Aug 22, 2021

This PR adds state.can(...):

someState.can({ type: 'TOGGLE' }); // `true` if it will cause a transition

@changeset-bot
Copy link

changeset-bot bot commented Aug 22, 2021

🦋 Changeset detected

Latest commit: 65c7e62

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

@davidkpiano davidkpiano changed the title Add state.can [core] Add state.can(event) Aug 22, 2021
@VanTanev
Copy link
Contributor

VanTanev commented Aug 23, 2021

If we're adding this in core, I would like to see the full functionality of https://github.com/VanTanev/xstate-helpers/blob/development/src/react/useIsXStateTransitionAvailable.ts - eg, type inference for both event types as strings and event objects - XState supports that in other places.

Also, I strongly believe that the result of state.can() should always be a boolean and never undefined. If there is no machine available on the state, can() should just return false.

Also, what is the best practice around memorizing this? Is machine.transition() expensive enough to want to use @xstate/react/useSelector() around state.can()?

@VanTanev
Copy link
Contributor

VanTanev commented Aug 23, 2021

Patch attached:

diff --git a/packages/core/src/State.ts b/packages/core/src/State.ts
index 17435556..0378407a 100644
--- a/packages/core/src/State.ts
+++ b/packages/core/src/State.ts
@@ -129,7 +129,9 @@ export class State<
    */
   public children: Record<string, ActorRef<any>>;
   public tags: Set<string>;
-  private machine: StateMachine<TContext, any, TEvent, any> | undefined;
+  protected machine:
+    | StateMachine<TContext, any, TEvent, TTypestate>
+    | undefined;
   /**
    * Creates a new State instance for the given `stateValue` and `context`.
    * @param stateValue
@@ -321,16 +323,16 @@ export class State<
   /**
    * Determines whether sending the `event` will cause a transition.
    *
-   * If this state was created outside of a machine (e.g., with `State.from(...)`), `undefined` will be returned.
+   * If this state was created outside of a machine (e.g., with `State.from(...)`), `false` will be returned.
    *
    * @param event The event to test
    * @returns Whether the event will cause a transition
    */
-  public can(event: TEvent): boolean | undefined {
+  public can(event: TEvent | TEvent['type']): boolean {
     if (!this.machine) {
-      return undefined;
+      return false;
     }
 
-    return this.machine.transition(this, event).changed;
+    return !!this.machine.transition(this, event).changed;
   }
 }
diff --git a/packages/core/test/state.test.ts b/packages/core/test/state.test.ts
index 8b56738f..70145a3f 100644
--- a/packages/core/test/state.test.ts
+++ b/packages/core/test/state.test.ts
@@ -590,6 +590,10 @@ describe('State', () => {
             on: {
               NO_CHANGE: 'a',
               ACTION: { actions: 'someAction' },
+              ACTION_WITH_COND: {
+                actions: 'someAction',
+                cond: (_: any, e: any) => e.cond
+              },
               CHANGE: 'b'
             }
           },
@@ -598,15 +602,30 @@ describe('State', () => {
       });
 
       expect(machine.initialState.can({ type: 'UNKNOWN' })).toEqual(false);
+      expect(machine.initialState.can('UNKNOWN')).toEqual(false);
+
       expect(machine.initialState.can({ type: 'NO_CHANGE' })).toEqual(false);
+      expect(machine.initialState.can('NO_CHANGE')).toEqual(false);
+
       expect(machine.initialState.can({ type: 'ACTION' })).toEqual(true);
+      expect(machine.initialState.can('ACTION')).toEqual(true);
+
       expect(machine.initialState.can({ type: 'CHANGE' })).toEqual(true);
+      expect(machine.initialState.can('CHANGE')).toEqual(true);
+
+      expect(
+        machine.initialState.can({ type: 'ACTION_WITH_COND', cond: true })
+      ).toEqual(true);
+      expect(
+        machine.initialState.can({ type: 'ACTION_WITH_COND', cond: false })
+      ).toEqual(false);
+      expect(machine.initialState.can('ACTION_WITH_COND')).toEqual(false);
     });
 
-    it('should return undefined for states created without a machine', () => {
+    it('should return false for states created without a machine', () => {
       const state = State.from('test');
 
-      expect(state.can({ type: 'ANY_EVENT' })).toEqual(undefined);
+      expect(state.can({ type: 'ANY_EVENT' })).toEqual(false);
     });
   });
 });

@VanTanev
Copy link
Contributor

After the last commit, this is chef's kiss~!
💯

Only thing left to fix is an incompatibility with StateConfig type.

It might be OK to make machine a public property?

@VanTanev
Copy link
Contributor

Oops, my bad for leaving the 2nd machine prop there, sorry.

'xstate': minor
---

You can now know if an event will cause a state change by using the new `state.can(event)` method, which will return `true` if the machine will change the state when sent the `event`, or `false` otherwise:
Copy link
Member

Choose a reason for hiding this comment

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

It would be great to explain what a change is. People could understand this differently. A change is when:

  • a state value changes
  • there are actions to be executed (entry/transition/exit)
  • context value changes

I hope I have not forgotten about anything

Copy link
Contributor

Choose a reason for hiding this comment

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

We probably should add the most common use-case: Checking buttons that execute transitions but are guarded by some condition.

Copy link
Contributor

Choose a reason for hiding this comment

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

💯 agree with both comments

Copy link
Member Author

Choose a reason for hiding this comment

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

Added to changeset. Will add the button example to the documentation in a subsequent PR

@Andarist
Copy link
Member

How should errors thrown by assign actions and guards be treated? How they should be treated when we implement error-related changes and we actually start capturing all unhandled errors? Should an error be counted as a change (errors will, most likely, try to select a transition - why way or another)?

@VanTanev
Copy link
Contributor

@Andarist I think there are two options here, both seem valid:

  1. try-catch around the .can() and return false on caught errors
  2. Do nothing, if .can() throws it's up to the caller to figure it out

@davidkpiano
Copy link
Member Author

How should errors thrown by assign actions and guards be treated? How they should be treated when we implement error-related changes and we actually start capturing all unhandled errors? Should an error be counted as a change (errors will, most likely, try to select a transition - why way or another)?

Can this be resolved in a separate PR? Let's keep this one low-scope.

@davidkpiano
Copy link
Member Author

@Andarist Ready for re-review

Comment on lines +585 to +587
describe('.can', () => {
it('should return true for a simple event that results in a transition to a different state', () => {
const machine = createMachine({
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add an example for when only the context changes?

Copy link
Member

Choose a reason for hiding this comment

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

This would be very same-ish to the one here https://github.com/statelyai/xstate/pull/2546/files#diff-c70d4083bd7a91fd4094f9f0a5925c5d4df0e268c78a40cf97e71f7787b8b300R667 . However, assign actions are treated in a special way in the core - so it would still be worth adding such a test 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Added!

Copy link
Contributor

@farskid farskid left a comment

Choose a reason for hiding this comment

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

Besides the comments, LGTM.

@Andarist
Copy link
Member

Andarist commented Sep 1, 2021

Can this be resolved in a separate PR? Let's keep this one low-scope.

At this stage, this is a design question about this API - both for v4 and for v5 (the answer can differ for both). I feel like it should be answered before merging this PR in because whatever lands on the main branch should be "releasable" and we shouldn't release something that might change (behavior-wise) soon-ish.

If you don't want to keep this PR low-scope but you'd like to make the design of this more tight & explicit before releasing then you could always create a PR targeting this PR.

If the answer is: "nothing should happen in v4 and errors should bubble to the caller" then it's fine. We can refine this in v5 (but a ticket~ should be created so we don't forget about it) but the appropriate tests should be added to validate this and to prevent us from regressing.

@davidkpiano
Copy link
Member Author

If the answer is: "nothing should happen in v4 and errors should bubble to the caller" then it's fine. We can refine this in v5 (but a ticket~ should be created so we don't forget about it) but the appropriate tests should be added to validate this and to prevent us from regressing.

This is the answer IMO. Just added a test for this 👍

How would this be refined in v5?

@Andarist
Copy link
Member

How would this be refined in v5?

That depends on the outcome of the error handling refactor. The behavior has been settled for the v4 though so let's just don't bother with thinking how this related to v5

@davidkpiano
Copy link
Member Author

@Andarist Ready for final review 🏁

Comment on lines +618 to +633
it('should return true for an event object that results in a new action', () => {
const machine = createMachine({
initial: 'a',
states: {
a: {
on: {
NEXT: {
actions: 'newAction'
}
}
}
}
});

expect(machine.initialState.can({ type: 'NEXT' })).toBe(true);
});
Copy link
Member

Choose a reason for hiding this comment

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

note - this is basically a duplicate of

it('should return true for a targetless transition with actions', () => {
const machine = createMachine({
initial: 'a',
states: {
a: {
on: {
EV: {
actions: () => {}
}
}
}
}
});
expect(machine.initialState.can({ type: 'EV' })).toBe(true);
});

@davidkpiano davidkpiano merged commit c78ef98 into main Sep 10, 2021
@davidkpiano davidkpiano deleted the davidkpiano/can branch September 10, 2021 14:23
@github-actions github-actions bot mentioned this pull request Sep 10, 2021
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.

4 participants