Ember Data: Return Promise from DS.Model.save()

[fivetanley]

Rendered

[btecu]

This is great!

[luxzeitlos]

In my opinion.save() is not really the problem. The primary problem are relations, followed by findAll/findRecord/query/queryRecord. And to solve them we need a way to work seamlessly with computed properties, templates and promises. There are different ideas and solutions like await helpers. I even have my own little helper for computed properties. However I think we need something official.

And I'm not sure if I like the idea to change the result of save at a different time then the result of relationships and findAll/findRecord/query/queryRecord. This just creates two different APIs, but does not really solve the problem. Its easier if save just returns the same as findRecord.

[runspired]

@luxferresum

We should not prevent progress in one area because of a preference for fixing issues other areas.

In my opinion.save() is not really the problem.

Implementation problems with save (in addition to the usage issues mentioned in the RFC)

• We return a new unique PromiseProxy for every call to save, regardless of whether the save call creates a new request (or even any request at all).
• These proxies are leaks (untracked, and never destroyed), which cause mysterious app and test failures when the backing record is destroyed or unloaded.
• Older versions of Ember (pre 3.1) cause these leaked proxies to blow up with errors if we properly destroy the record instance.

The primary problem are relations, followed by findAll/findRecord/query/queryRecord. And to solve them we need a way to work seamlessly with computed properties, templates and promises. There are different ideas and solutions like await helpers. I even have my own little helper for computed properties. However I think we need something official.

Solutions for issues with finders are en-route, but doing it all at once would be far more disruptive.

And I'm not sure if I like the idea to change the result of save at a different time then the result of relationships and findAll/findRecord/query/queryRecord. This just creates two different APIs, but does not really solve the problem. Its easier if save just returns the same as findRecord.

Many other record APIs return promises or the record instead of a PromiseProxy. With the exception of save, we only return a PromiseProxy today when we may not have the record available already. We do have it available during save.

[igorT]

This looks great to me as well. @runspired we should discuss this at the next meeting if we haven't already

[rwjblue]

Chatted about this in todays ember-data meeting, the general consensus is that we'd like to move forward.

[hjdivad]

Discussed during today's meeting; we've already agreed this looks like a good path forward; putting it in FCP.

[pete-the-pete]

COOL https://github.com/pete-the-pete/data/tree/promise-after-save <-- hopefully that can come in handy.

…

On Wed, Aug 21, 2019 at 2:23 PM David J. Hamilton ***@***.***> wrote: Discussed during today's meeting; we've already agreed this looks like a good path forward; putting it in FCP. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#362?email_source=notifications&email_token=AACUPAL3OCUOCLRPYBC4K4DQFWW5HA5CNFSM4FRBVJUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD43FO3A#issuecomment-523655020>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACUPAJAZBDXLUFZE66BDDLQFWW5HANCNFSM4FRBVJUA> .

[HeroicEric]

I think this might be a good opportunity to do the same refactoring for Model.reload(), which also currently returns a PromiseObject for all of the same reasons mentioned in this RFC.

https://github.com/emberjs/data/blob/5e495394cbf67025be2622f604e67004d79bc21c/packages/store/addon/-private/system/model/model.js#L984-L996

[Gaurav0]

This has been in FCP for months?

[runspired]

After discussion today we decided that the return value here needs to be a custom thenable, basically a Promise with three additional properties for knowing the state

isPending
isResolved
isRejected

We are otherwise 👍 on merging this

[knownasilya]

Ping

[runspired]

I'll mention at the next meeting that we forgot to merge, we should go ahead and merge :)

[snewcomer]

We could also implement something on top of Promise that exposes these flags.

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

[snewcomer]

I guess their method could be wrapped with something similar to this or ember-concurrency as well. Almost every save we deal with uses derived state to help the UI along and prevent user initiated bugs.

So I take that back. Returning a simple Promise is better.

[snewcomer]

Will start working on this one! Will have to figure out how to update to get CI to pass. Or we just dup it to a new RFC.

[snewcomer]

supplanted by #795!