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

Best way to avoid invoking promises multiple times (or keeping context up to date) #124

Open
KidkArolis opened this issue Jul 31, 2020 · 2 comments

Comments

@KidkArolis
Copy link

KidkArolis commented Jul 31, 2020

Hi, thanks for a great library, been enjoying learning to use it.

I don't know how common this is, but I want to keep various bits of data (props, data from other hooks) in the context and I want to keep them up to date even if I'm in a middle of invocation.

To do so, I've added an 'assign' transition to every state:

const close = ctx => ctx.onClose()
const assign = (ctx, { type, ...data }) => ({ ...ctx, ...data })
const assignable = name => transition('assign', name, reduce(assign))

const machine = createMachine({
    initialising: state(
      immediate('configure', guard(loadingDone)),
      immediate('initialisingFailed', guard(loadingError)),
      transition('close', 'closing', action(close)),
      assignable('initialising'),
    ),
    initialisingFailed: state(
      transition('close', 'closing', action(close)),
      assignable('initialisingFailed'),
    ),
    configure: state(
      transition('close', 'closing', action(close)),
      transition('save', 'saving'),
      assignable('configure'),
    ),
    saving: invoke(
      save,
      transition('done', 'closing', action(close)),
      transition('error', 'configure', reduce(assign)),
      transition('close', 'closing', action(close)),
      assignable('saving'), // <--- a problem! because transitioning back to itself will invoke again
    ),
    closing: state(),
  })

A few thoughts / questions / learnings from this so far:

  1. Having to manually add assignable to each state is a bit tedious, but not too bad. Given that you might always want to keep context up to date wrt to external data (e.g. onClose function passed via prop, or some other bits of context), wondering if there should be an easier way to update the context (in xstate, I think you can do that by handling an even in the root machine).
  2. My assignable helper seems "unidiomatic", because I have to specify the name of the state I'm adding the transition to. There is no way to transition to self without knowing the name of the state. and so I have to pass the name of the state to each assignable call. Wondering if self transitions could be made easier.. Or perhaps the first problem is solved in a different way, this would also go away.

I'm not really saying these are even a problem, I think it's good to be explicit and keep the rule set small and simple.

But the next bit is more challenging. The issue is that if you're in the saving state and send 'assign' event to update context (if say the parent component rerendered passing a new onClose prop), you invoke the save function again, but we don't want that in this case of self transition. Now, the best way I found so far to avoid this was to create a custom invokeOnce helper, which only invokes the function on entering the state and not on "self" transitions:

const valueEnumerable = value => ({ enumerable: true, value })
const create = (a, b) => Object.freeze(Object.create(a, b))
const invokePromiseOnceType = {
  enter(machine, service, event) {
    const name = machine.current
    const prev = service.machine.current
    if (prev !== name) {
      this.fn
        .call(service, service.context, event)
        .then(data => service.send({ type: 'done', data }))
        .catch(error => service.send({ type: 'error', error }))
    }

    return machine
  },
}

export function invokeOnce(fn, ...transitions) {
  const s = state(...transitions)
  return create(invokePromiseOnceType, {
    fn: valueEnumerable(fn),
    transitions: valueEnumerable(s.transitions),
  })
}

Only sharing all this to get feedback from any current / future users of robot about how they handle this sort of stuff.

For example, would it be better if invoke always triggered on enter only, and to allow invoking multiple times you'd have to transition out and back in?

Or should there be a way to guard an invoke? (not sure that's semantically correct).

Or perhaps another way would to solve this is to use an intermediate state:

    configure: state(
      transition('save', 'save'),
      assignable('configure'),
    ),
    save: invoke(
      save,
      immediate('saving')
    ),
    saving: state(
      transition('done', 'closing', action(close)),
      transition('error', 'configure', reduce(assign)),
      assignable('saving'), // no longer a problem, since we're no longer in an invoke state
    ),
@KidkArolis
Copy link
Author

KidkArolis commented Jul 31, 2020

Another idea.. what if invoke was more like action (part of a transition), except it also gets send passed in... in fact, you could get rid of invoke altogether if action received send, e.g.:

const save = (ctx, ev, send) => { // <-- notice send, a new param, enables async behaviours not just promises
  send({ type: 'assign, saving: true })
  post(ev.id, ev.data)
    .then((data) => { send({ type: 'done', data }) })
    .catch((error) => { send({ type: 'error', error }) })
}

const machine = machine({
  saving: state(
      immediate('saving', guard(ctx => !ctx.saving), action(save)),
      transition('done', 'closing', action(close)),
      transition('error', 'configure', reduce(assign)),
      transition('close', 'closing', action(close)),
      assignable('saving'),
    )
})

@gkiely
Copy link
Contributor

gkiely commented Dec 31, 2022

@KidkArolis

Just for reference Xstate supports this in the following way.

const machine = createMachine({
  states: {
    idle: {
      on: {
        load: 'loading',
      }
    },
    loading: {
      src: 'fetchData',
      onDone: 'idle',
      on: {
        assign: {
          actions: [
            // Perform updates during invocation
          ]
        }
      }
    },
  }
});

It also supports it via send in an invoked callback, but this way is more declarative.

I believe this could be addressed if the library allows action/reduce/guard as a second argument in response to an event.

const assign = (ctx, { type, ...data }) => ({ ...ctx, ...data })
const assignable = name => transition('assign', reduce(assign));

const machine = createMachine({
    saving: invoke(
      save,
      transition('done', 'closing', action(close)),
      transition('error', 'configure', reduce(assign)),
      transition('close', 'closing', action(close)),
      assignable('saving'), // <--- Works now
    ),
  })

A different name might be more fitting as it is no longer a transition and basically an event listener, on?

Update: got it working with the export onEvent https://github.com/gkiely/robot

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

No branches or pull requests

2 participants