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

[Feature] AbortController support for invoke #418

Closed
johnyanarella opened this issue Apr 10, 2019 · 8 comments
Closed

[Feature] AbortController support for invoke #418

johnyanarella opened this issue Apr 10, 2019 · 8 comments
Labels
enhancement 💬 RFC Request for comments
Milestone

Comments

@johnyanarella
Copy link

johnyanarella commented Apr 10, 2019

Bug or feature request?

Feature

Description:

Integrate support for AbortController/AbortSignal into service invocation.

Potential implementation:

Rather than tracking Promise service and callback service cancellation internally via a canceled Boolean, each invocation would instantiate an AbortController instance, and would call controller.abort() rather than toggling canceled = false. Promise resolution or rejection after the controller is aborted (signal.aborted === false) would be ignored. Attempts by a service function to execute callback() would similarly be ignored.

The controller.signal would be passed along as a property of a new optional third parameter, options, for the service function. Within the supplied service function, a developer could pass that signal along to the fetch() API's options parameter, or could inspect signal.aborted between steps of sequential async operations to bail out early when appropriate:

const machine = Machine({
  // ...
  states: {
    // ...
    loading: {
      invoke: {
        id: 'getUser',
        src: (ctx, event, {signal}) => {
          return fetch(`/user/${ctx.userId}`, {signal})
            .then(res => res.json())
        },
        onDone: {
          target: 'success',
          actions: assign({ user: (ctx, event) => event.data })
        },
        onError: {
          target: 'failure',
          actions: assign({ error: (ctx, event) => event.data })
        }
      }
    },
    // ...
  }
});

I have already implemented this locally via a reusable wrapper around my service functions, but it would be much more convenient to move that responsibility into XState.

Currently, my wrapper must implement the entire (context, event) => (callback, onEvent) signature in order to return the controller.abort() as part of a cleanup function. This means that I have to re-implement most of the existing logic for handling promise and callback services within that wrapper. This also requires the service id be passed as a parameter to the wrapper so that I can create an appropriate done.invoke.<id> event when needed.

So, while it is possible to integrate AbortController externally from XState, it's non-trivial and probably not very future-proof.

The tricky part in making this functionality available via XState is: how do we address environments that do not include AbortController? The API for it is extremely small, so I'd be inclined to use it when present, and use an alternative (but local) implementation when it is not. From the perspective of the API we make public, we can make the visible surface area (the signal) effectively equivalent for both scenarios.

Curious what you think. Thanks!

@davidkpiano davidkpiano added enhancement 💬 RFC Request for comments labels Apr 10, 2019
@jatwork
Copy link

jatwork commented May 1, 2019

While implementing a search-as-you-type kind of feature (where there will be multiple requests issued one after the other, and each one cancels the previous one), I realized this is indeed a very much needed functionality, huge bump for clearly drafting it, and hopefully it will be realized soon!

@davidkpiano davidkpiano added this to the 4.7 milestone Jun 6, 2019
@aavelyn
Copy link

aavelyn commented Jul 14, 2019

bump +1 !!
I'm currently implementing a paginated list machine with seach and sort features. A cancellation feature is absolutely necessary :)

@johnyanarella
Copy link
Author

Good to hear this would be helpful for others, too.

Sorry I hadn't gotten back to this yet - I've had a user land implementation (i.e. outside of XState) of this working locally for quite a while, so this fell off my radar.

However, my external approach turned out to be a bit brittle. Since the doneInvoke and error actions are not exported, I was previously working around that by simulating the corresponding DoneEvent and ErrorPlatformEvent in my code. However, XState 4.6.1+ included a fix where it was necessary to change the shape of the error event, which briefly broke my code. Serves me right for relying on simulating some undocumented XState internals 😬!

So, the good news is that gives me some incentive to get back to this ticket with a proper PR to potentially get this functionality integrated into XState!

I'll try to make some time for that this week!

@rostero1
Copy link
Contributor

rostero1 commented Sep 13, 2019

I tried this a while back and and could only come up with a solution that relied on transient transitions, but it wouldn't work because of the way assign actions are raised:

Since assign() actions are raised, the context is updated before other actions are executed. This means that other actions within the same step will get the updated context rather than what it was before the assign() action was executed.

This meant that actions that should happen after were happening before the actions linked to the prior transaction. I was trying to abort and then renew the controller, but instead it would renew the controller and abort it.

I'm curious to see if anyone has come up with a solution since that time.

Here's the simplified example provided by another user on gitter:
https://codesandbox.io/s/cranky-shaw-v3z6h

Imagine logFromAtoB is abort. incrementAndLog and logFromBtoA creates the new abort controller and does the fetch.

@Andarist
Copy link
Member

I was thinking about it a bit and seems to me that this could be very nicely implemented in userland by using callbacks & composition.

Right now, as mentioned by @johnyanarella, this can be done - only it requires constructing doneInvoke event on your own. It's a limitation of exported APIs - but it can be easily fixed by just exporting this helper.

If we could consider refactoring this in the future major version we could go even further. XState on its own doesn't really need to know about any "complex" types - everything can be considered just a fancy-wrapped callbacks, as everything can be built on top of that primitive. I highly encourage watching https://www.youtube.com/watch?v=HssczgaY9BM and https://www.youtube.com/watch?v=fdol03pcvMA

Basically what we could do is to provide a callback primitive and wrappers around it - this would give us a great composability, while keeping XState simple.

POC implementation
Interpreter.prototype.spawn(spawnAction, id) {
  const { type, create } = spawnAction
  let canceled = false;

  const listeners = new Set();

  const receive = event => {
    listeners.forEach(listener => listener(event))

    if (canceled) {
      return;
    }

    this.send(event);
  };

  const complete = (data, isError) => {
    // TODO: construct different event shape for `isError` case
    const event = doneInvoke(id, data)
    receive(event)
  }
  
  const handler = create(receive, complete);

  const actor = {
    id,
    type,
    send: event => {
      if (isFunction(handler)) {
        handler(event)
      }
    },
    subscribe: next => {
      listeners.add(next);

      return {
        unsubscribe: () => {
          listeners.delete(next);
        }
      };
    },
    stop: () => {
      canceled = true;
      
      if (isFunction(handler)) {
        handler(undefined, true);
      }
    },
    toJSON() {
      return { id };
    }
  };

  this.children.set(id, actor);

  return actor;
}
spawnPromise built on top of that
const spawnPromise = promiseCreator => spawn('promise', (_send, complete) => {
    let canceled = false
    promiseCreator().then(
      result => {
        if (!canceled) {
          complete(result)
        }
      },
      error => {
        if (!canceled) {
          complete(error, true)
        }
      },
    )
    return (_event, done) => {
      if (done === true) {
          canceled = true
          return
      }
    }
})
spawnObservable built on top of that
const spawnObservable = observableCreator => spawn('observable', (send, complete) => {
    const observable = observableCreator()

    const subscription = observable.subscribe(
      value => {
        send(value)
      },
      error => {
        complete(error, true)
      },
      () => {
        complete()
      }
    )
    return (_event, done) => {
      if (done === true) {
        subscription.unsubscribe()
      }
    }
})
spawnAbortableFetch built on top of that
const spawnAbortableFetch = fetchCreator => spawn('abortable_fetch', (send, complete) => {
    const controller = new AbortController();
    const signal = controller.signal;

    fetchCreator({ signal }).then(
      result => {
        complete(result)
      },
      error => {
        complete(error, true)
      },
    )

    return (_event, done) => {
      if (done === true) {
        controller.abort()
      }
    }
})
spawnCallback built on top of that
const spawnCallback = callbackCreator => spawn('callback', callbackCreator)

While this might look more verbose at first, it gives few benefits:

  • smaller bundle sizes
  • extensibility
  • aids visualization by providing spawned actor type explicitly.

One thing I haven't solved in this POC API is that we need to have ability to pass context, event and stuff to creators. Like - in this shape this could be used if we'd expect invoke to return a spawn action, but that wouldn't help visualizing stuff (as the spawned type wouldn't be known until invoke would get called/resolved). Solving this shouldn't be that hard though, we'd only have to consider different possible solutions and their tradeoffs.

@gkiely
Copy link

gkiely commented Sep 5, 2022

@davidkpiano Would you consider re-opening this?

The userland example provided is quite verbose and passing an abortable fetch seems like a reasonable use case for Xstate to handle.

Alternatively, documenting the correct approach for an abortable fetch would also be useful.

@Andarist
Copy link
Member

Andarist commented Sep 5, 2022

In v5 you will be able to implement (we might even provide it out of the box) fromAbortable. If you are interested in this feature in v4 then I would recommend wrapping this logic in such a helper for the time being. Right now you should be able to implement it like this:

const fromAbortable = (create) =>
  createMachine({
    context: () => ({
      controller: new AbortController(),
    }),
    exit: ({ controller }) => controller.abort(),
    initial: "loading",
    states: {
      loading: {
        invoke: {
          src: ({ controller }) => create({ signal: controller.signal }),
          onDone: 'done',
          onError: { actions: escalate((_, { data }) => data) }
        },
      },
      done: {
        type: 'final',
        data: (_, { data }) => data
      }
    },
  });

Note that I've just put a PoC together here and I didn't actually test this out.

@cmh-echomark
Copy link

Thank you for the help @Andarist !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 💬 RFC Request for comments
Projects
None yet
Development

No branches or pull requests

8 participants