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

[bug] Immediate doesn't trigger onChange event #102

Closed
kybarg opened this issue Mar 19, 2020 · 13 comments · Fixed by #109
Closed

[bug] Immediate doesn't trigger onChange event #102

kybarg opened this issue Mar 19, 2020 · 13 comments · Fixed by #109
Assignees
Labels
bug Something isn't working

Comments

@kybarg
Copy link
Contributor

kybarg commented Mar 19, 2020

const three = createMachine({
  threeInit: state(
    immediate('threeDone')
  ),
  threeDone: state(),
})

const two = createMachine({
  twoInit: state(
    immediate('twoStart'),
  ),
  twoStart: invoke(three,
    transition('done', 'twoDone'),
  ),
  twoDone: state(),
})

const one = createMachine({
  oneInit: state(
    transition('START', 'oneStart'),
  ),
  oneStart: invoke(two,
    transition('done', 'oneDone'),
  ),
  oneDone: state(),
})
const interpretedMachine = interpret(one, (service) => {
  console.log(service.machine.current)
});

interpretedMachine.send('START')

Output

oneStart

Expected output

oneStart
twoInit
twoStart
threeInit
threeDone
twoDone
oneDone
@matthewp
Copy link
Owner

Is this the same thing as what is described here? #69

The issue is that the interpret callback only gets called when events are sent into the service. So you can check the service returned by interpret to see what the current state is.

@kybarg
Copy link
Contributor Author

kybarg commented Mar 19, 2020

@matthewp don't think so as when I call

console.log(interpretedMachine.machine.current)
console.log(interpretedMachine.child.machine.current)
console.log(interpretedMachine.child.child.machine.current)

I get output

oneStart
twoStart
threeDone

so seems like it is stuck on the machine three but it should have cycled back

@matthewp
Copy link
Owner

Ok, makes sense, it just sounded similar to that issue. I'll probably be able to take a look this weekend.

@matthewp matthewp added the bug Something isn't working label Mar 19, 2020
@kybarg
Copy link
Contributor Author

kybarg commented Mar 19, 2020

@matthewp thanks, also found that if I add intermediate state to machine three with transition to done and trigger that by send it cycles back but runs into problem covered here #100

@matthewp
Copy link
Owner

Looked into it a bit this morning. Temporarily got worried that we couldn't transition past 2 states in one step, but luckily that's not the problem.

I found a fix that requires making nested machines asynchronous, but I want to avoid forcing async if at all possible, so still working on it.

@matthewp matthewp self-assigned this Mar 21, 2020
@matthewp
Copy link
Owner

Uh, this highlights the need to get rid of services. One problem is that a service holds on to the current state until the transition is completely done. This means we can't synchronously complete a child machine in the same turn as we enter that machine.

I really want to get rid of services, created #106 to that effect. Still will try to find some solution to this in the meantime.

@matthewp
Copy link
Owner

This is my working branch (mostly posting this so I remember): https://github.com/matthewp/robot/tree/imm-nested

@kybarg
Copy link
Contributor Author

kybarg commented Mar 23, 2020

@matthewp your solution doesn't work

import { createMachine, immediate, state, transition, invoke, interpret } from 'robot3'

const three = createMachine({
    threeInit: state(
        immediate('threeDone')
    ),
    threeDone: state(),
}, null, 'three')

const two = createMachine({
    twoInit: state(
        immediate('twoStart'),
    ),
    twoStart: invoke(three,
        transition('done', 'twoDone'),
    ),
    twoDone: state(),
}, null, 'two')

const one = createMachine({
    oneInit: state(
        transition('START', 'oneStart'),
    ),
    oneStart: invoke(two,
        transition('done', 'oneDone'),
    ),
    oneDone: state(),
})

const interpretedMachine = interpret(one, (service) => {
    // console.log('current', service.machine.current)
});

interpretedMachine.send('START')
console.log(interpretedMachine.machine.current)

Result:

oneStart

Expected result

oneDone

@kybarg
Copy link
Contributor Author

kybarg commented Mar 23, 2020

So I would say the problem is not even with onChange, rather the way enterImmediate works

@matthewp
Copy link
Owner

That branch doesn't have a fix. I haven't implemented a fix yet. I have a test which I think encapsulates the problem but I'll make sure to test your examples when a fix is ready as well.

@matthewp
Copy link
Owner

@kybarg I have a fix in this branch: https://github.com/matthewp/robot/tree/imm-nested

If you have time and want to test this against what you're doing that would be appreciated. I'm going to try and refactor it a little bit to reduce the file size as much as possible.

@kybarg
Copy link
Contributor Author

kybarg commented Mar 23, 2020

@matthewp now works like a charm

@matthewp
Copy link
Owner

matthewp commented Apr 2, 2020

Released as part of https://github.com/matthewp/robot/releases/tag/v0.2.15

Sorry for the wait, and thanks a lot for finding this one, this was an important bug to fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants