[WIP] Fixes actions on same-state transitions#75
[WIP] Fixes actions on same-state transitions#75good-idea wants to merge 1 commit intoMicheleBertoli:masterfrom
Conversation
MicheleBertoli
left a comment
MicheleBertoli
left a comment
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.
| 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.
| 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.
| 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 :) |
2 participants
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.