ember-data: Return a Promise from Model.save()

[snewcomer]

Partial dup of #362 (comment) with improvements. Specifically the addition of derived state to the promise wrapper.

rendered

[sandstrom]

Would it make more sense to return a vanilla Promise?

Most of RSVP is already available in native Promise.

(I'm guessing RSVP will eventually be deprecated too)

[snewcomer]

@sandstrom I like your suggestion. Might be something we want to RFC to get off of RSVP now that we are past IE11. I'll talk to Chris tomorrow!

[snewcomer]

There are a few hairy parts where we were relying on private RSVP API's to guard against dangling promises. Specifically here...So alongside this PR, we are going to introduce a deprecation for RSVP.

[runspired]

Added final comment period, planning on merging monday considering we previously moved this same RFC to FCP in #362

That said I would like to revisit the decision to add the state-flags. I think there's a strong case for it being a separate util, even if an official one. @rwjblue @hjdivad @igorT thoughts?

Ignore the specific package name, this is just hand-wave sketch, but if you have thoughts on package name to give them: This would more or less look something like:

class PromiseState extends Promise {
  @tracked isRejected = false;
  @tracked isResolved = false;
  @tracked isPending = true;
}

// in JS (promiseState would weakmap to a PromiseState to provide stable return for same promise)
import { promiseState } from "@ember-data/template-utils";

//...

const state = promiseState(promise);
//...
if (state.isPending) {

}

{{!-- use without caching off or without JS component having exposed the promise-state --}}
{{#if (get (promise-state savePromise) "isPending")}}
{{/if}}

{{!-- use with caching --}}
{{#let (promise-state savePromise) as |state|}}
  {{#if state.isPending}}
  {{else if state.isResolved}}
  {{else if state.isRejected}}
  {{/if}}
{{/let}}

[jelhan]

That said I would like to revisit the decision to add the state-flags. I think there's a strong case for it being a separate util, even if an official one.

I agree that an util function would be way better - especially for teaching. But I don't see how such an util function could be implemented. As far as I know there is no way to synchronously determine a JavaScript Promise's state.

[snewcomer]

@jelhan There has been a few approaches I have taken (the first being pulled from promise many array implementation).

https://github.com/emberjs/data/pull/7868/files#diff-8aca4274066862134d43e99891338fd7bbd479fbb186cf0ab2f721fc55436a37

https://github.com/snewcomer/ember-stateful-promise/blob/main/addon/utils/stateful-promise.js

I'm unsure of what these two examples don't cover or if you were thinking about something else. One downside to the util approach is for users to fish though their code to replace. However, in conjunction with #796, I could see us not concerning ourselves with the derived state.

[jelhan]

@jelhan There has been a few approaches I have taken (the first being pulled from promise many array implementation).

https://github.com/emberjs/data/pull/7868/files#diff-8aca4274066862134d43e99891338fd7bbd479fbb186cf0ab2f721fc55436a37

https://github.com/snewcomer/ember-stateful-promise/blob/main/addon/utils/stateful-promise.js

I'm unsure of what these two examples don't cover or if you were thinking about something else. One downside to the util approach is for users to fish though their code to replace. However, in conjunction with #796, I could see us not concerning ourselves with the derived state.

Thanks a lot for sharing this examples. I used the ember-stateful-promise library as an example to show the issue: snewcomer/ember-stateful-promise#30

The state of a Promise, which has been resolved before constructing a StatefulPromise, is reported wrongly as isRunning until next tick. As far as I know this limitation is by design and can not be fixed until ECMAScript provides an API to synchronously determine the state of a Promise.

I think that's the reason why @runspired proposes to have a WeakMap to provide a stable return for the same promise. That would resolve the issue of having two different states reported for the same promise - depending if the stateful promise instance state was created before or after the Promise settled. But the state may still be wrong for one tick.

I fear that this API limitation would be very confusing to consumers. Let me illustrate using one very confusing edge case based on the API proposed by @runspired in #795 (comment):

const promise = post.save();
await promise;

const state = promiseState(promise);
console.log('isPending', state.isPending); // true
console.log('isResolved', state.isResolved); // false

// do something async, which is totally unrelated,
// but causes later code to be executed in the next tick
await Promise.resolve();

console.log('isPending', state.isPending); // false
console.log('isResolved', state.isResolved); // true

🤯

[snewcomer]

@jelhan Really nice points. Just a few notes...

1. The stateful-promise export is a promise, so I would expect it would be isRunning until the next tick. A promise within a promise of sorts.

2. I can see how the latter example you provided could be an issue. But in practice with async programming, more often than not, wouldn't be.

const statefulPromise = promiseState(promise);
await unrelated promise;
const result = await statefulPromise;

result // correct!

But I guess it would really trip people up if they used those state flags for non UI purposes.

const statefulPromise = promiseState(promise);
await unrelated promise;

if (statefulPromise.isRunning) {
  const result = await statefulPromise;
  result // correct!
}

So I agree with your take and runspireds.

[runspired]

@jelhan while it is true that potential for it to be wrong by one-tick exists, the WeakMap solution effectively solves that problem for the 99% case.

const promise = record.save();
const state = promiseState(promise);
// all state flags are correct before the await
await promise;
// all state flags are still correct after the await
// all state 2 flags are immediately correct, because state2 === state
const state2 = promiseState(promise);

For the 1% case where someone fails to call promiseState until after the promise has resolved, there are two outcomes:

1. the incorrect pending state is non-consequential since it gets fixed on the next tick (I suspect the majority of 1% cases would fall into this bucket since if the state mattered that much it likely would have been requested immediately after the promise was created instead of much later)
2. the incorrect pending state matters but is easily addressed by ensuring promiseState is called on the outcome since that was what was desired.