[WIP] Fixes actions on same-state transitions #75
Conversation
There was a problem hiding this comment.
Thank you very much for submitting this PR, @good-idea.
I'm all-in to get this merged, and I left a few comments.
For now, it really seems value and actions is what we need to check to detect changes, and I agree that a transition to an invalid state should work as it does now.
| @@ -30,3 +30,12 @@ export const matches = (patterns, value) => { | |||
|
|
|||
| return patterns.some(pattern => values.some(val => pattern.test(val))) | |||
| } | |||
|
|
|||
| export const machineDidUpdate = (oldMachine, newMachine) => { | |||
There was a problem hiding this comment.
I think this should be:
(prevMachineState.value !== machineState.value) || machineState.actions.length
Otherwise, this would be true when the previous machine has actions, and the current one doesn't (i.e. it's invalid).
I would also replace machine with state or, better, with machineState, and rename it to something like hasMachineStateChanged.
| @@ -118,7 +119,7 @@ const withStateMachine = (statechart, options = {}) => Component => { | |||
|
|
|||
| handleComponentDidUpdate(prevProps, prevState) { | |||
| this.isTransitioning = false | |||
|
|
|||
| /* WIP - Should this logic be the same as what is used in handleTransition? */ | |||
There was a problem hiding this comment.
Yep, it definitely should.
| @@ -30,3 +30,12 @@ export const matches = (patterns, value) => { | |||
|
|
|||
| return patterns.some(pattern => values.some(val => pattern.test(val))) | |||
| } | |||
|
|
|||
| export const machineDidUpdate = (oldMachine, newMachine) => { | |||
There was a problem hiding this comment.
Although this is pretty simple, I would add a couple of tests.
| @@ -105,6 +111,7 @@ test('state', () => { | |||
| test('actions', () => { | |||
There was a problem hiding this comment.
I wouldn't repurpose this test, but I suggest we create a separate one (like "invalid transition with extended state") or, add a new case to "render".
|
Ping @good-idea :) |
Just giving this a shot. This makes a 'fix' to work as I'd expect (see #74) but now there is a failing test.
Needs (from my perspective):
[ ]
machineDidUpdateutility probably needs to be improved to better detect changes[ ] Transitioning to a nonexistent state (
transition(FOO)) should act as it did before.Let me know if this is helpful, I'm happy to continue working on this. If not, I can just call
transitionwith an empty object.