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

immediate does not callback in interpret #69

Closed
JNavith opened this issue Oct 8, 2019 · 9 comments
Closed

immediate does not callback in interpret #69

JNavith opened this issue Oct 8, 2019 · 9 comments
Labels
discussion Discussing ideas surrounding Robot enhancement New feature or request

Comments

@JNavith
Copy link

JNavith commented Oct 8, 2019

Hello! I've been loving this library but I discovered some unexpected behavior with immediate (long post ahead).

Original code snippet:

import { createMachine, immediate, interpret, invoke, reduce, state, state as final, transition } from 'robot3';

const wait = (ms) => new Promise(resolve => setTimeout(resolve, ms));
const load = async () => {
    await wait(1000);
    return 'content';
};

const machine = createMachine({
    ready: state(immediate("loading")),
    loading: invoke(load, transition('done', 'loaded')),
    loaded: final(),
});

console.log(`state: ${machine.current}`);

interpret(machine, service => {
    console.log(`state: ${service.machine.current}`);
});

Expected:

  • state: ready
  • state: loading
  • 1 second passes
  • state: loaded

What actually happens:

  • state: ready
  • 1 second passes
  • state: loaded

The state machine never actually has the loading state when transitioned to from an immediate state! All other kinds of transitions to loading would actually have loading be the state rather than skipping over it (I've actually had to retitle and rewrite this post a few times because I keep misidentifying what the pattern / problem is with the interaction of invoke and immediate).

One implication here is that, if you are using this to render a UI, your application can't know from the state machine that that 1 second is spent in loading; it'll still show its ready state. You would have to add a side effect (in the load function) that reaches outside the state machine to inform the outside world that there is loading taking place.

Without changing how immediate works (i.e. in the current version of the library), you can use a recipe like this to get my originally expected behavior (at least in this situation; I haven't tested it in the infinite other ways you could):

// all the codes from the original snippet except for the machine definition
const immediately = () => wait(0);

const machine = createMachine({
    ready: invoke(immediately, transition('done', 'loading')),
    loading: invoke(load, transition('done', 'loaded')),
    loaded: final(),
});
// same interpreter from the original snippet

However, this has problems like

  • requiring those helper functions (wait and immediately) to be written in userspace
  • leaving immediate without any purpose (as far as I'm aware?)

I think some solutions could be

  1. if this is the intended behavior: documenting that immediate works this way very explicitly
  2. if this is unintended behavior but not wanting to make a breaking change: having the library provide a shortcut like export const immediately = next => invoke(() => wait(0), transition('done', next));
  3. if this is unintended behavior that should be fixed with a breaking change: rewrite immediate to work in some way based off of the function in 2.
@matthewp
Copy link
Owner

matthewp commented Oct 8, 2019

What I think is happening here is this: When you call interpret(machine) the machine is immediately transitioned to the loading state. It doesn't call the callback (maybe it should?) but it is in the loading state. If you look at the service returned by interpret(machine) I think it will show you what has happened.

let service = interpret(machine, () => {
   // later, service.machine.current; // loaded
});

// service.machine.current; // loading?

I understand that this is not super intuitive. I don't love the interpret() API and would like to find something better.

@matthewp matthewp added question Further information is requested and removed question Further information is requested labels Oct 8, 2019
@JNavith JNavith changed the title bug / confusing behavior with immediate and invoke immediate does not callback in interpret Oct 11, 2019
@matthewp
Copy link
Owner

@SirNavith Does this make sense now?

@JNavith
Copy link
Author

JNavith commented Oct 14, 2019

Sorry for the late reply.
Yes, you're right. In this snippet:

import { createMachine, immediate, interpret, invoke, reduce, state, state as final, transition } from 'robot3';

const wait = (ms) => new Promise(resolve => setTimeout(resolve, ms));

const machine = createMachine({
  start: state(transition("rainbow", "red")),
  red: state(immediate("orange")),
  orange: state(immediate("yellow")),
  yellow: state(immediate("green")),
  green: state(immediate("blue")),
  blue: state(immediate("indigo")),
  indigo: state(immediate("violet")),
  violet: state(immediate("end")),
  end: state(transition("restart", "start")),
});

async function main() {
  console.log(`state: ${machine.current}`);

  const { send } = interpret(machine, service => {
    console.log(`state: ${service.machine.current}`);
  });	
  
  await wait(100);
  send("rainbow");
  
  await wait(100);
  send("restart");
  
  await wait(100);
  send("rainbow");
}

main();

The state machine will cycle through all of the colors, but the callback in interpret won't be run for these states. If you were to check the state of the service (service.machine.current) during this time, it would correctly show the color, even though it didn't inform the callback there was a change (i.e. all that gets logged is state: start or state: end).

So, I've changed the title of this post to reflect that, because I think this is a bug.

I understand that this is not super intuitive. I don't love the interpret() API and would like to find something better.

Not to tell you how to manage your own project or anything but maybe you could make another issue for this where you describe what you've been thinking and maybe I could comment there?

@matthewp
Copy link
Owner

matthewp commented Oct 14, 2019

No problem @SirNavith, happy to have the discussion. What is it you are wanting to do with the temporary state in the interpret callback? Usually that callback is used to update the UI when the new state has settled. If you were to update the UI in this case it would immediately be updated again, the user would never see the temporary state. If the purpose is for logging there is action for that: https://thisrobot.life/api/action.html

Maybe there's another use-case that I haven't thought of, so let me know.

@matthewp matthewp added enhancement New feature or request discussion Discussing ideas surrounding Robot labels Oct 14, 2019
@JNavith
Copy link
Author

JNavith commented Oct 15, 2019

The snippet in the original post shows a use case:

One implication here is that, if you are using this to render a UI, your application can't know from the state machine that that 1 second is spent in loading; it'll still show its ready state. You would have to add a side effect (in the load function) that reaches outside the state machine to inform the outside world that there is loading taking place.

@matthewp
Copy link
Owner

interpret() returns a service, so you can check that to know that the it is in the loading state.

let service = interpret(machine, () => {
    console.log(`state: ${service.machine.current}`);
});

console.log(service.machine.current); // "loading"

@JNavith
Copy link
Author

JNavith commented Oct 18, 2019

Yes, you can check that on demand, but then you would have to poll for state changes instead of being "given" the value when it changes, which is the point of onChange in interpret. Any example on the project's homepage that depends on one of robot's first-class integrations (which all depend on robot-hooks who uses onChange in interpret) would not work for any immediate states, which is especially problematic (as in the original example) for any immediate transition to an invoke state with a promise.

The main point is that immediate has different and unexpected behavior from other states and transitions because it won't alert the onChange callback given to interpret (the "service"), which I would consider a bug.

I am limited in JS literacy so I cannot figure out how this bug affects immediate without affecting transition because it looks like immediate wraps transition's behavior. Then I thought to rewrite immediate using invoke, but I think this would not allow for multiple guards or reduces. So I'm back to trying to understand what's different about immediate in the codebase that makes it not trigger an onChange call while transition does. I'll reply back if I figure anything out.

@matthewp
Copy link
Owner

Before changing Robot's code do you want to write up an example in a codepen of something that you think won't work? The service returned by interpret() has the correct current state as shown here.

You don't need to poll for the state change. What you do need to do is use the service returned by interpret() to draw the UI appropriate for the first time. After that any other state changes will go through the interpret callback.

Like I said, if you want to create a codepen / codesandbox of what you think won't work I'd be happy to get it working for you.

@JNavith
Copy link
Author

JNavith commented Mar 13, 2020

I'll be closing this issue now:

  1. No one else feels the same way I do, or has come across this and thought it was weird like I did at first, otherwise they would have commented here about it
  2. I finally created a project in Preact using the official preact-robot integration to test the original example, and it correctly showed the loading state as expected (though this is in disagreement with what I'd been seeing with interpret so I don't know how the robot-hooks-based integrations are able to overcome this)
  3. The original problem can be worked around by putting a dummy state before ready, when it comes to the interpret route (i.e. the Preact solution didn't need this) (also this is if I remember what I did correctly)
  4. My opinion about this being unexpected behavior has changed, since the examples I tried to write again today (i.e. with a fresh mind since it's been almost 5 months since the last activity on this issue) seemed reasonable to me

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Discussing ideas surrounding Robot enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants