xstate icon indicating copy to clipboard operation
xstate copied to clipboard

[v5] Resumable promise logic

Open davidkpiano opened this issue 2 years ago • 10 comments
trafficstars

This PR introduces a step(...) wrapper to persist intermediate state in promises.

The step(name, promiseFn) function executes the promise created by the promiseFn the first time, and persists its result to the actor's internal state.

The promise actor's state can then be persisted (actor.getPersistedState()) and restored normally (createActor(logic, { state })) and the logic will essentially "resume" at the latest incomplete step(...). The way this works is by using persisted values for previous steps; if there is a persisted value, no need to run the promise again.

const flow = fromPromise(async ({ step }) => {
  const user = await step('user', () => fetchUser());

  const friends = await step('friends', () => fetchFriends(user.id));

  if (friends.length > 0) {
    const party = await step('plan party', () => createEvent(user, friends));
    return party;
  }

  return null;
});

Motivation

This would be completely opt-in - you can create promise logic as normal. Calling async functions in a named step(name, promiseFn) gives you a couple benefits:

  • Simple visualization - we can show a basic partial state machine for previously executed steps
  • Persistence - we can persist the results of each step (make sure they're uniquely named!) so that when we restore a promise, it can fast-forward to where you left off
  • Observability - when hooked up to Stately in the future, you can see the status/result of each step visualized on a sequence diagram

davidkpiano avatar Aug 12 '23 20:08 davidkpiano

⚠️ No Changeset found

Latest commit: 637446eb693fc9097e57b8634ceace4e6082af01

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

changeset-bot[bot] avatar Aug 12 '23 20:08 changeset-bot[bot]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 637446eb693fc9097e57b8634ceace4e6082af01:

Sandbox Source
XState Example Template Configuration
XState React Template Configuration

codesandbox-ci[bot] avatar Aug 12 '23 20:08 codesandbox-ci[bot]

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

ghost avatar Aug 12 '23 20:08 ghost

I really like this idea, but I think it's going to be very easy to forget to use step() somewhere, and that would have really unpleasant consequences when trying to resume a function. TypeScript can't help either, as there's no way to constrain what can be awaited. If this used generators instead it would be possible to ensure that step() is always used, both at runtime and when type checking.

phpnode avatar Aug 14 '23 23:08 phpnode

could we remove first parameter of step? how do you think about using reference check of promise as React's useThenable?

ellemedit avatar Aug 15 '23 05:08 ellemedit

I really like this idea, but I think it's going to be very easy to forget to use step() somewhere, and that would have really unpleasant consequences when trying to resume a function. TypeScript can't help either, as there's no way to constrain what can be awaited. If this used generators instead it would be possible to ensure that step() is always used, both at runtime and when type checking.

You have the same problem with generators, since it's possible to make a promise.then(...) call without wrapping it or execute some unwrapped side-effect.


The important thing to remember here is that this is an opt-in feature. The default behavior is that the promise (or parts of the promise) will re-run. So it will still work, but this may be undesirable. However, I'd argue that it's impractical to prevent every possible way that a developer may forget to do something or do things wrongly, outside of providing lint rules.

IMO if strict guardrails are desired, using a state machine would be the way to go.

davidkpiano avatar Aug 15 '23 11:08 davidkpiano

could we remove first parameter of step? how do you think about using reference check of promise as React's useThenable?

If we remove the first parameter, then 1) we don't have explicit names for steps, which makes it harder to visualize, and 2) similar to React hooks, we're now dependent on the order in which these steps get called, which limits the ability to call these within e.g. loops or if-statements.

Reference checking wouldn't work when restoring state (e.g. from localstorage or a DB).

davidkpiano avatar Aug 15 '23 11:08 davidkpiano

If we remove the first parameter, then 1) we don't have explicit names for steps

Maybe name could be inferred from named functions, and overridden by name attribute?

function step(promiseCreator: () => Promise<any>, nameOverride?: string) {
  let name = promiseCreator.name;
  if (nameOverride) name = nameOverride;
  
  if(!name) {
    if (isDevelopment) {
      console.warn('...')
    }
    return promiseCreator();
  }
        
  if (state.steps?.[name]) {
    return state.steps[name][1];
  }

  ...
}

Usage:

const fetchUser = () => fetch(url);
step(fetchUser); // => name = fetchUser
step(fetchUser, 'user'); // => name = user
step(() => fetch(url), 'user'); // => name = user
step(() => fetch(url)); // => no name, just returns the promise and warns in dev mode

pixtron avatar Aug 15 '23 13:08 pixtron

I guess depending 'name' property of constructor or instance could make confusion for production as names are mingled formally?

ellemedit avatar Aug 15 '23 16:08 ellemedit

I guess depending 'name' property of constructor or instance could make confusion for production as names are mingled formally?

That might indeed be problematic for visualization.

pixtron avatar Aug 15 '23 20:08 pixtron