From 3b9f02e9178dc4de8d9d2ec8d462703febd8e9a7 Mon Sep 17 00:00:00 2001 From: Stanley Stuart Date: Wed, 22 Aug 2018 14:04:22 -0700 Subject: [PATCH 1/2] Ember Data: Return Promise from DS.Model.save() --- ...-data-return-promise-from-ds-model-save.md | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 text/0000-ember-data-return-promise-from-ds-model-save.md diff --git a/text/0000-ember-data-return-promise-from-ds-model-save.md b/text/0000-ember-data-return-promise-from-ds-model-save.md new file mode 100644 index 0000000000..7421e289a2 --- /dev/null +++ b/text/0000-ember-data-return-promise-from-ds-model-save.md @@ -0,0 +1,65 @@ +* Start Date: 08/22/2018 +* RFC PR: (leave this empty) +* Ember Issue: (leave this empty) + +# Return a Promise from DS.Model.save() + +## Summary + +DS.Model.save() will return an Ember.RSVP.Promise instead of a PromiseObject. + +## Motivation + +* The API documentation already documents the return value as a Promise. +* Remove dependency on promise proxies +* Async Consistency - The PromiseObject encourages a usage pattern of sometimes-async, sometimes-sync behavior. We want to [reduce zalgo](http://blog.izs.me/post/59142742143/designing-apis-for-asynchrony). +* API consistency - While the PromiseObject proxies properties, the PromiseObject does not proxy methods on the underlying model. For instance, if you try to call a method like destroyRecord on the model, you will get a “not a function” error because the method is called on the Proxy and not the underlying model. This encourages users for Ember Data to reach into private implementation details of the promise proxy, such as using proxy.get('content') to call methods. +* Enable new functionality in Ember Data while making backwards compatibility with older versions of Ember easier. +* The documentation of using PromiseProxy APIs has been replaced over time in the guides and API documentation to use more typical JavaScript usage of Promises, such as async/await, instead of PromiseProxies. + +## Detailed design + +Introduce a new feature flag, “ds-return-promise-from-model-save”. When enabled, DS.Model.save() will return a RSVP.Promise that resolves with the model if the save was successful, or rejects with an error if the save fails. When disabled, a PromiseObject will be returned to keep today's behavior, but accessing properties on the PromiseObject or calling non-Promise functions (.then, .finally, .catch) on the PromiseObject will issue a deprecation warning. + +Deprecation Plan: + +The deprecations can be implemented by adding deprecations around the existing PromiseObject. However, we can't include the deprecations in the actual PromiseObject class because [it is the base class for the relationship proxies](https://github.com/emberjs/data/blob/master/addon/-legacy-private/system/promise-proxies.js#L85). Instead, we can create and use a new DeprecatedPromiseObject for the DS.Model.save() function. It functions like a PromiseObject but deprecates the following behavior: + + * Accessing properties on the PromiseObject returned from DS.Model.save() + * Use deprecateProperty from Ember to deprecate all the properties that the PromiseProxy Mixin provides. The list of properties can be found here: https://github.com/emberjs/ember.js/blob/master/packages/ember-runtime/lib/mixins/promise_proxy.js + * Deprecate accessing any unknown properties. For instance, someone might try to grab the value of an attr or computed property from a model, e.g. proxy.get('email'). + * Calling functions that are not available on RSVP.Promises + * Not Safe (issue deprecation): .get/.set/anything else really + * Safe (do not issue a deprecation): .then, .catch, .finally + +The implementation follows the standard feature-flagging process in Ember and Ember Data. The final implementation will look something like: + +``` + save(options) { + const savePromise = this._internalModel.save(options).then(() => this); + + if (flagEnabled("ds-return-promise-from-model-save")) { + return savePromise; + } else { + return DeprecatedPromiseObject.create({ + promise: savePromise + }); + } + }, +``` + +## How we teach this + +We do not believe this requires any update to the Ember curriculum. API documentation does not need to be updated as it is already documented as returning a promise. This behavior of accessing properties through the PromiseProxyMixin specifically for DS.Model.save() is not documented in the guides. + +## Drawbacks + +For users relying on this behavior, they will have to refactor their code to either use patterns like async/await, ember-concurrency, or ember-promise-helpers. Alternatively, if all they want to access are properties on the model, they can use the model instead. + +## Alternatives + +The impact of not doing this prevents further changes in Ember Data and requires more effort to maintain compatibility between Ember Data and older versions of Ember. + +## Unresolved questions + +* It's quite possible Ember Data users use the isSettled, isRejected, isFulfilled, and isPending properties of the PromiseProxyMixin to display state in their UI. For example, you may have a component that calls record.save(), and uses isPending to show a loading spinner or isRejected to show UI for error states. Should we point them to community alternatives like [ember-promise-helpers](https://github.com/fivetanley/ember-promise-helpers) or [ember-concurrency](http://ember-concurrency.com/docs/introduction/) in the deprecation message for these properties? From c284dc308fd74a1133c1e9345ac63f8adb2277ee Mon Sep 17 00:00:00 2001 From: Ricardo Mendes Date: Thu, 23 Aug 2018 12:42:05 +0100 Subject: [PATCH 2/2] Update 0000-ember-data-return-promise-from-ds-model-save.md --- text/0000-ember-data-return-promise-from-ds-model-save.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0000-ember-data-return-promise-from-ds-model-save.md b/text/0000-ember-data-return-promise-from-ds-model-save.md index 7421e289a2..fd22f2de7b 100644 --- a/text/0000-ember-data-return-promise-from-ds-model-save.md +++ b/text/0000-ember-data-return-promise-from-ds-model-save.md @@ -6,7 +6,7 @@ ## Summary -DS.Model.save() will return an Ember.RSVP.Promise instead of a PromiseObject. +DS.Model.save() will return an RSVP.Promise instead of a PromiseObject. ## Motivation @@ -19,7 +19,7 @@ DS.Model.save() will return an Ember.RSVP.Promise instead of a PromiseObject. ## Detailed design -Introduce a new feature flag, “ds-return-promise-from-model-save”. When enabled, DS.Model.save() will return a RSVP.Promise that resolves with the model if the save was successful, or rejects with an error if the save fails. When disabled, a PromiseObject will be returned to keep today's behavior, but accessing properties on the PromiseObject or calling non-Promise functions (.then, .finally, .catch) on the PromiseObject will issue a deprecation warning. +Introduce a new feature flag, “ds-return-promise-from-model-save”. When enabled, DS.Model.save() will return an RSVP.Promise that resolves with the model if the save was successful, or rejects with an error if the save fails. When disabled, a PromiseObject will be returned to keep today's behavior, but accessing properties on the PromiseObject or calling non-Promise functions (.then, .finally, .catch) on the PromiseObject will issue a deprecation warning. Deprecation Plan: