2

I have a typescript class with a save method, and I want the next call to the save method, to only happen once the first one is completed. Imagine the following case:

  count = 0;
  async save() {
      let x = this.count++;
      console.log("start " + x);
      await axios.post("***",{});
      console.log("end " + x);
  }
}

In this case - when the user calls save, without awaiting it - the second post can be called before the first one completed - causing all sorts of problems.

The solution that I came up with is:

  lastSave = Promise.resolve();
  count = 0;
  async save() {
    this.lastSave = this.lastSave.then(async () => {
      let x = this.count++;
      console.log("start " + x);
      await axios.post("***",{});
      console.log("end " + x);
    });
  }

Is this a valid solution, or is there a better way?

Noam
  • 4,472
  • 5
  • 30
  • 47
  • https://stackoverflow.com/questions/50269671/when-to-use-promise-over-observable (I tended to use Observables more - because of power of being able to cancel queued up events. Not sure what your goal is here). https://rxmarbles.com/ – JGFMK Jan 02 '22 at 11:42
  • It's certainly valid (as in the code works) but I suppose it depends how much control you need over the process i.e. you can't cancel any of these saves once they've been called. As the other commenter suggested, you could use the Observable pattern, or even fairly easily implement your own promise queue so that each save remains a discrete event you can interact with and if necessary prevent before it actually gets called. – lawrence-witt Jan 02 '22 at 11:50
  • All of this is possible easily with promises and there is no need for observables for simple things like retry or cancellation (using the cross-platform built-in AbortController). – Benjamin Gruenbaum Jan 02 '22 at 13:37
  • "*causing all sorts of problems*" - like what? – Bergi Jan 02 '22 at 14:00

1 Answers1

6

This pattern of thening the last promise is entirely valid. The only issue with what you currently have is error handling. In your current code if one request fails it will fail all future requests.

A more complete solution would be something like:

  lastSave = Promise.resolve();
  count = 0;
  async save() {
    const savePromise = this.lastSave.then(async () => {
      let x = this.count++;
      console.log("start " + x);
      await axios.post("***",{});
      console.log("end " + x);
    });
    // wait but don't fail forever on errors
    this.lastSave = savePromise.then(() => {}).catch(() => {});
    // You also need to return the promise so callers can react
    // to it. Note the lack of `.catch` here to not interfere with error handling
    return await savePromise; 
  }
Benjamin Gruenbaum
  • 270,886
  • 87
  • 504
  • 504
  • I agree that `save()` should return a promise so that the call can be awaited, but shouldn't the `async` keyword before `save` be removed then? – md2perpe Jan 02 '22 at 18:29
  • @md2perpe that `async` and `await` are only in the `then` callback and only for the `console.log`s – Benjamin Gruenbaum Jan 02 '22 at 19:14
  • I didn't talk about `.then(async () => { ... await ... })` but of `async save() {`. Shouldn't that be just `save() {`? – md2perpe Jan 02 '22 at 19:18
  • @md2perpe it's a small difference, if it's `async` the stack trace will show `save` in the stack trace if there are errors (since return is an await point). – Benjamin Gruenbaum Jan 02 '22 at 19:30
  • Actually let me make it explicit – Benjamin Gruenbaum Jan 02 '22 at 19:31
  • The added `await` makes a difference. Before, `await save()` would return a promise (`savePromise`) which should also be awaited. – md2perpe Jan 02 '22 at 19:51
  • How would you best incapsulate this? Let's say I need it in more than one place in my code - I'm curious as to the best way to refactor this - to as simple as possible. – Noam Jan 03 '22 at 06:48
  • Put it in a high order function and keep the last state in its closure instead of on the object I guess then change to `save = sequence(...)`? – Benjamin Gruenbaum Jan 03 '22 at 08:40
  • @md2perpe No, if a Promise resolves to a Promise or other "thenable" (as it would when you return a Promise in an `async` function), it's automatically unwrapped. There might be a slight benefit to awaiting in terms of the clarity of the stack trace, but functionally it's identical whether or not you `await`. See [this Fiddle](https://jsfiddle.net/yoctmbxh/). – Jeff Bowman Jan 03 '22 at 19:53
  • @JeffBowman. Now I remember that I have read that. That's a reason promises are not true monads. – md2perpe Jan 03 '22 at 20:04
  • 1
    @BenjaminGruenbaum This is a very reasonable solution, but its users should know that it will repeat: if you call `save` three times it will make three server calls, even if the second call makes the third obsolete. One might expect this of an `increment` server call, but maybe not `save` or `refresh`. – Jeff Bowman Jan 03 '22 at 20:25
  • 1
    @JeffBowman I absolutely agree and it's a good point that any time we add this sort of synchronization we are significantly making the code harder to debug and reason about (which may or may not be worth it). – Benjamin Gruenbaum Jan 04 '22 at 12:03