From a41cffac185bf88bb960be9ad77309e9982b46c9 Mon Sep 17 00:00:00 2001 From: Yuri Ratanov Date: Fri, 11 Feb 2022 19:10:01 +0700 Subject: [PATCH 1/7] Polymorphic relations without inheritance --- ...ymporphic-relations-without-inheritance.md | 135 ++++++++++++++++++ 1 file changed, 135 insertions(+) create mode 100644 text/0000-polymporphic-relations-without-inheritance.md diff --git a/text/0000-polymporphic-relations-without-inheritance.md b/text/0000-polymporphic-relations-without-inheritance.md new file mode 100644 index 0000000000..6b8b341911 --- /dev/null +++ b/text/0000-polymporphic-relations-without-inheritance.md @@ -0,0 +1,135 @@ +--- +Stage: Accepted +Start Date: +Release Date: Unreleased +Release Versions: + ember-source: vX.Y.Z + ember-data: vX.Y.Z +Relevant Team(s): +RFC PR: +--- + + + +# + +## Summary + +Allow setting polymorph relations without inheritance and mixins. + +## Motivation + +In pre-octane Ember you could set polymorphic relations using two ways: inheritance or mixins. Mixins are deprecated and impossible to use with native classes, so the only option is to use inheritance: + +```js +import Model, { belongsTo, hasMany } from '@ember-data/model'; + +class TagModel extends Model { + @belongsTo('taggable', { polymorphic: true }) taggable; +} + +// not a model, just a way to make relations work +class TaggableModel extends Model { + @hasMany('tag', { inverse: 'taggable' }) tags; +} + +// real models with tags +class CommentModel extends TaggableModel {} +class PostModel extends TaggableModel {} +``` + +Which fells apart when you need your model to be a target of several polymorphic relations. + +```js +import Model, { belongsTo, hasMany } from '@ember-data/model'; + +class ViewModel extends Model { + @belongsTo('viewable', { polymorphic: true }) viewable; +} + +class ViewableModel extends Model { + @hasMany('views', { inverse: 'viewable' }) views; +} + +// How to make Comment and Post viewable?... +``` + +You can do some dirty tricks making it inherit from each other `ViewableModel extends TaggableModel`. Or even by adding new base model class, like this: + +```js + +import Model, { belongsTo, hasMany } from '@ember-data/model'; + +class ModelWithPolymorphic extends Model {} + +class TagModel extends Model { + @belongsTo('model-with-polymorphic', { polymorphic: true }) taggable; +} + +class ViewModel extends Model { + @belongsTo('model-with-polymorphic', { polymorphic: true }) viewable; +} + +class CommentModel extends ModelWithPolymorphic { + @hasMany('views', { inverse: 'viewable' }) views; + @hasMany('tag', { inverse: 'taggable' }) tags; +} + +``` + +While it solves the problem, it seems like a misuse of inheritance and too much trickery involved. + +## Detailed design + +I propose to introduce new API, similar to what [rails have](https://guides.rubyonrails.org/association_basics.html#polymorphic-associations). + +```js +import Model, { belongsTo, hasMany } from '@ember-data/model'; + +class TagModel extends Model { + @belongsTo('taggable', { polymorphic: true }) taggable; +} + +class ViewModel extends Model { + @belongsTo('viewable', { polymorphic: true }) viewable; +} + +class CommentModel extends Model { + @hasMany('views', { inverse: 'viewable' }) views; + @hasMany('tag', { inverse: 'taggable' }) tags; +} +``` + +`taggable` and `viewable` in this example should be typed as `extends Model`, so any model can take that spot. +JSON:API has enough information (`type` and `id`) to set this relation. + +## How we teach this + +> Would the acceptance of this proposal mean the Ember guides must be +re-organized or altered? Does it change how Ember is taught to new users +at any level? + +The docs should be changed to promote the new way. + +## Drawbacks + +None + +## Alternatives + +> What other designs have been considered? What is the impact of not doing this? + +> This section could also include prior art, that is, how other frameworks in the same domain have solved this problem. + +## Questions + +It might be considered to use `as` instead of `inverse` to make it stand out as polymorphic relation, but if it works without it, I think it's better to keep `inverse`. From 614ab9b11d44ebba08836ad685a1aa4da4220a81 Mon Sep 17 00:00:00 2001 From: Yuri Ratanov Date: Fri, 11 Feb 2022 19:14:26 +0700 Subject: [PATCH 2/7] rename file --- ...d => 0793-polymporphic-relations-without-inheritance.md} | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) rename text/{0000-polymporphic-relations-without-inheritance.md => 0793-polymporphic-relations-without-inheritance.md} (96%) diff --git a/text/0000-polymporphic-relations-without-inheritance.md b/text/0793-polymporphic-relations-without-inheritance.md similarity index 96% rename from text/0000-polymporphic-relations-without-inheritance.md rename to text/0793-polymporphic-relations-without-inheritance.md index 6b8b341911..c41bd3e1da 100644 --- a/text/0000-polymporphic-relations-without-inheritance.md +++ b/text/0793-polymporphic-relations-without-inheritance.md @@ -6,7 +6,7 @@ Release Versions: ember-source: vX.Y.Z ember-data: vX.Y.Z Relevant Team(s): -RFC PR: +RFC PR: https://github.com/emberjs/rfcs/pull/793 --- -# +# Polymorphic relations without inheritance ## Summary From 3620aeef0576e9dc86e3edf1115f3d23862e2c07 Mon Sep 17 00:00:00 2001 From: Yuri Ratanov Date: Mon, 21 Feb 2022 08:40:03 +0700 Subject: [PATCH 3/7] Update text/0793-polymporphic-relations-without-inheritance.md Co-authored-by: Chris Thoburn --- text/0793-polymporphic-relations-without-inheritance.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0793-polymporphic-relations-without-inheritance.md b/text/0793-polymporphic-relations-without-inheritance.md index c41bd3e1da..bcd4a462de 100644 --- a/text/0793-polymporphic-relations-without-inheritance.md +++ b/text/0793-polymporphic-relations-without-inheritance.md @@ -90,7 +90,7 @@ While it solves the problem, it seems like a misuse of inheritance and too much ## Detailed design -I propose to introduce new API, similar to what [rails have](https://guides.rubyonrails.org/association_basics.html#polymorphic-associations). +This RFC proposes to allow explicitly declaring polymorphic traits similar to [rails conventions](https://guides.rubyonrails.org/association_basics.html#polymorphic-associations). ```js import Model, { belongsTo, hasMany } from '@ember-data/model'; From 6b4f1b9f2225d6a47554339caa828b0e5beaafdb Mon Sep 17 00:00:00 2001 From: Yuri Ratanov Date: Mon, 21 Feb 2022 20:33:08 +0700 Subject: [PATCH 4/7] apply feedback --- ...ymporphic-relations-without-inheritance.md | 36 +++++-------------- 1 file changed, 8 insertions(+), 28 deletions(-) diff --git a/text/0793-polymporphic-relations-without-inheritance.md b/text/0793-polymporphic-relations-without-inheritance.md index bcd4a462de..4d7796263d 100644 --- a/text/0793-polymporphic-relations-without-inheritance.md +++ b/text/0793-polymporphic-relations-without-inheritance.md @@ -24,11 +24,12 @@ RFC PR: https://github.com/emberjs/rfcs/pull/793 ## Summary -Allow setting polymorph relations without inheritance and mixins. +1. Allow setting polymorph relations without inheritance and mixins. +2. Deprecate using inheritance and mixins for polymorphic relations. Target version: 5.0. ## Motivation -In pre-octane Ember you could set polymorphic relations using two ways: inheritance or mixins. Mixins are deprecated and impossible to use with native classes, so the only option is to use inheritance: +You could set polymorphic relations using two ways: inheritance or mixins. ```js import Model, { belongsTo, hasMany } from '@ember-data/model'; @@ -47,47 +48,26 @@ class CommentModel extends TaggableModel {} class PostModel extends TaggableModel {} ``` -Which fells apart when you need your model to be a target of several polymorphic relations. +Now consider that this same post and comment class also both need to be "viewable", and that "viewable" is a trait also shared by other models that are not "taggable". Today this is only achievable via mixins. ```js import Model, { belongsTo, hasMany } from '@ember-data/model'; -class ViewModel extends Model { - @belongsTo('viewable', { polymorphic: true }) viewable; -} - -class ViewableModel extends Model { - @hasMany('views', { inverse: 'viewable' }) views; -} - -// How to make Comment and Post viewable?... -``` - -You can do some dirty tricks making it inherit from each other `ViewableModel extends TaggableModel`. Or even by adding new base model class, like this: - -```js - -import Model, { belongsTo, hasMany } from '@ember-data/model'; - -class ModelWithPolymorphic extends Model {} - class TagModel extends Model { - @belongsTo('model-with-polymorphic', { polymorphic: true }) taggable; + @belongsTo('taggable', { polymorphic: true }) taggable; } class ViewModel extends Model { - @belongsTo('model-with-polymorphic', { polymorphic: true }) viewable; + @belongsTo('viewable', { polymorphic: true }) viewable; } -class CommentModel extends ModelWithPolymorphic { - @hasMany('views', { inverse: 'viewable' }) views; +class CommentModel extends Model.extends(TaggableMixin, ViewableMixin) { @hasMany('tag', { inverse: 'taggable' }) tags; + @hasMany('views', { inverse: 'viewable' }) views; } ``` -While it solves the problem, it seems like a misuse of inheritance and too much trickery involved. - ## Detailed design This RFC proposes to allow explicitly declaring polymorphic traits similar to [rails conventions](https://guides.rubyonrails.org/association_basics.html#polymorphic-associations). From c7e8636164938c98835178e9960c30bf5b7e3c90 Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Wed, 13 Apr 2022 15:41:14 -0700 Subject: [PATCH 5/7] Expand RFC Details --- ...ymporphic-relations-without-inheritance.md | 83 +++++++++++++++++-- 1 file changed, 74 insertions(+), 9 deletions(-) diff --git a/text/0793-polymporphic-relations-without-inheritance.md b/text/0793-polymporphic-relations-without-inheritance.md index 4d7796263d..7f6587923d 100644 --- a/text/0793-polymporphic-relations-without-inheritance.md +++ b/text/0793-polymporphic-relations-without-inheritance.md @@ -52,12 +52,13 @@ Now consider that this same post and comment class also both need to be "viewabl ```js import Model, { belongsTo, hasMany } from '@ember-data/model'; +import Mixin from "@ember/object/mixin"; -class TagModel extends Model { +class TaggableMixin extends Mixin { @belongsTo('taggable', { polymorphic: true }) taggable; } -class ViewModel extends Model { +class ViewableMixin extends Mixin { @belongsTo('viewable', { polymorphic: true }) viewable; } @@ -72,33 +73,95 @@ class CommentModel extends Model.extends(TaggableMixin, ViewableMixin) { This RFC proposes to allow explicitly declaring polymorphic traits similar to [rails conventions](https://guides.rubyonrails.org/association_basics.html#polymorphic-associations). +### API + +The inverse relationship, if one exists, of a polymorphic relationship must be concrete (e.g. not also polymorphic). + +The polymorphic relationship **MUST** explicitly declare itself as polymorphic and **MUST** explicitly declare it's inverse as either a key or `null`. All records that satisfy the polymorphic type must declare a matching inverse via that key. + +Polymorphic relationships take the form: + +\```ts +hasMany(baseType: string, options: { polymorphic: true, inverse: string | null }) +belongsTo(baseType: string, options: { polymorphic: true, inverse: string | null }) +\``` + +Concrete Relationships which fulfill polymorphic relationships take the form: + +\```ts +hasMany(recordWithPolymorphicRelationship: string, options: { inverse: string, as: string }) +belongsTo(recordWithPolymorphicRelationship: string, options: { inverse: string, as: string }) +\``` + +So for instance, given a field named "tagged" that is a polymorphic hasMany with baseType "taggable", and inverse key "tags" the following would be the polymorphic relationship and concrete relationship. + +\```ts +// polymorphic relationship +class Tag extends Model { + @hasMany("taggable", { polymorphic: true, inverse: "tags" }) tagged; +} +// an inverse concrete relationship +class Post extends Model { + @hasMany("tag", { inverse: "tagged", as: "taggable" }) tags; +} +\``` + +### Example + ```js import Model, { belongsTo, hasMany } from '@ember-data/model'; class TagModel extends Model { - @belongsTo('taggable', { polymorphic: true }) taggable; + @belongsTo('taggable', { polymorphic: true, inverse: 'tag' }) taggable; } class ViewModel extends Model { - @belongsTo('viewable', { polymorphic: true }) viewable; + @belongsTo('viewable', { polymorphic: true, inverse: 'views' }) viewable; } class CommentModel extends Model { - @hasMany('views', { inverse: 'viewable' }) views; - @hasMany('tag', { inverse: 'taggable' }) tags; + @hasMany('views', { inverse: 'viewable', as: 'viewable' }) views; + @hasMany('tag', { inverse: 'taggable', as: 'taggable' }) tags; } ``` `taggable` and `viewable` in this example should be typed as `extends Model`, so any model can take that spot. JSON:API has enough information (`type` and `id`) to set this relation. +### Polymorphic Relationships without Inverses + +- polymorphic relationships need not have an inverse, in which case they must specify `inverse: null`. + +### Base Types + +Every polymorphic relationship has an implicit "base type" that represents the entity on the other side of the relationship. + +In the below example, the polymporphic relationship implies the existence of the `base type` "taggable". + +\``` +class Tag extends Model { + @hasMany("taggable", { inverse: "tags", polymorphic: true }) taggables; +} +\``` + +It is only required that your app implement a model (or if using instantiateRecord and the schema service implement handling for that type) if your API will return entities whose type is the base type. If the base type is abstract and never directly interacted with, it is not required to implement a Model or other associated support. + +### Limitations + +- polymorphic to polymorphic relationships are not allowed. One side of the relationship MUST be a concrete entity type or have no inverse. +- this includes self-referential relationships + +### Deprecation Design + +Situations in which polymorphism is not configured by the above mechanism but in which EmberData would have previously detected and attempted to do the right thing will be deprecated. This means effectively that mixin based and inheritance based polymorphism will print a deprecation *only when* the corresponding relationships are not also configured for polymorphism correctly. By this means users are free to continue using mixins and inheritance if they so choose. + ## How we teach this > Would the acceptance of this proposal mean the Ember guides must be re-organized or altered? Does it change how Ember is taught to new users at any level? -The docs should be changed to promote the new way. +Polymorphism was never explicitly documented or supported by EmberData. With this RFC, we would add an official declaration of support and add a section on polymorphism to both the docs for model, docs for the schema service and to the guides. ## Drawbacks @@ -106,10 +169,12 @@ None ## Alternatives -> What other designs have been considered? What is the impact of not doing this? +We could require a more explicit API that declares the full-intent and full-resolvability on one or even both sides. The advantage of both sides is both less work to do on the part of the ember-data to determine validity and increased clarity to the reader in that both sides show the config. + +However, This can have some drawbacks in that as an API grows to have more entities that satisfy a polymorphic relationship the config becomes increasingly and unnecessarily large and prevents runtime additions to the schema. > This section could also include prior art, that is, how other frameworks in the same domain have solved this problem. ## Questions -It might be considered to use `as` instead of `inverse` to make it stand out as polymorphic relation, but if it works without it, I think it's better to keep `inverse`. +None From 521f4af3a570f6ba7f0e9156386e6aedb52bf083 Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Wed, 13 Apr 2022 15:48:52 -0700 Subject: [PATCH 6/7] Update 0793-polymporphic-relations-without-inheritance.md --- ...ymporphic-relations-without-inheritance.md | 56 +++++++------------ 1 file changed, 21 insertions(+), 35 deletions(-) diff --git a/text/0793-polymporphic-relations-without-inheritance.md b/text/0793-polymporphic-relations-without-inheritance.md index 7f6587923d..579febbae6 100644 --- a/text/0793-polymporphic-relations-without-inheritance.md +++ b/text/0793-polymporphic-relations-without-inheritance.md @@ -1,11 +1,11 @@ --- Stage: Accepted -Start Date: +Start Date: 2022-02-11 Release Date: Unreleased Release Versions: ember-source: vX.Y.Z ember-data: vX.Y.Z -Relevant Team(s): +Relevant Team(s): Ember Data RFC PR: https://github.com/emberjs/rfcs/pull/793 --- @@ -24,12 +24,18 @@ RFC PR: https://github.com/emberjs/rfcs/pull/793 ## Summary -1. Allow setting polymorph relations without inheritance and mixins. -2. Deprecate using inheritance and mixins for polymorphic relations. Target version: 5.0. +1. Allow setting polymorphic relationships without inheritance and mixins. +2. Deprecate using inheritance and mixins for polymorphic relationships. Target version: 5.0. ## Motivation -You could set polymorphic relations using two ways: inheritance or mixins. +Currently, although undocumented and not explicitly supported, you can set polymorphic +relationships using two mechanisms: inheritance and mixins. This RFC seeks to lay out +an explicitly supported path for polymorphic relationships that composes well with +native class usage, eliminates the dependency on instanceof checks and prototype chain +walking, and is compatible with the schema service introduced in [RFC #487](https://github.com/emberjs/rfcs/blob/master/text/0487-custom-model-classes.md#exposing-schema-information) + +**Example Using Inheritance** ```js import Model, { belongsTo, hasMany } from '@ember-data/model'; @@ -48,7 +54,9 @@ class CommentModel extends TaggableModel {} class PostModel extends TaggableModel {} ``` -Now consider that this same post and comment class also both need to be "viewable", and that "viewable" is a trait also shared by other models that are not "taggable". Today this is only achievable via mixins. +**Example via Mixins** + +Consider that this same post and comment class also both need to be "viewable", and that "viewable" is a trait also shared by other models that are not "taggable". Today this is only achievable via mixins. ```js import Model, { belongsTo, hasMany } from '@ember-data/model'; @@ -81,21 +89,21 @@ The polymorphic relationship **MUST** explicitly declare itself as polymorphic a Polymorphic relationships take the form: -\```ts +```ts hasMany(baseType: string, options: { polymorphic: true, inverse: string | null }) belongsTo(baseType: string, options: { polymorphic: true, inverse: string | null }) -\``` +``` Concrete Relationships which fulfill polymorphic relationships take the form: -\```ts +```ts hasMany(recordWithPolymorphicRelationship: string, options: { inverse: string, as: string }) belongsTo(recordWithPolymorphicRelationship: string, options: { inverse: string, as: string }) -\``` +``` So for instance, given a field named "tagged" that is a polymorphic hasMany with baseType "taggable", and inverse key "tags" the following would be the polymorphic relationship and concrete relationship. -\```ts +```ts // polymorphic relationship class Tag extends Model { @hasMany("taggable", { polymorphic: true, inverse: "tags" }) tagged; @@ -104,30 +112,8 @@ class Tag extends Model { class Post extends Model { @hasMany("tag", { inverse: "tagged", as: "taggable" }) tags; } -\``` - -### Example - -```js -import Model, { belongsTo, hasMany } from '@ember-data/model'; - -class TagModel extends Model { - @belongsTo('taggable', { polymorphic: true, inverse: 'tag' }) taggable; -} - -class ViewModel extends Model { - @belongsTo('viewable', { polymorphic: true, inverse: 'views' }) viewable; -} - -class CommentModel extends Model { - @hasMany('views', { inverse: 'viewable', as: 'viewable' }) views; - @hasMany('tag', { inverse: 'taggable', as: 'taggable' }) tags; -} ``` -`taggable` and `viewable` in this example should be typed as `extends Model`, so any model can take that spot. -JSON:API has enough information (`type` and `id`) to set this relation. - ### Polymorphic Relationships without Inverses - polymorphic relationships need not have an inverse, in which case they must specify `inverse: null`. @@ -138,11 +124,11 @@ Every polymorphic relationship has an implicit "base type" that represents the e In the below example, the polymporphic relationship implies the existence of the `base type` "taggable". -\``` +``` class Tag extends Model { @hasMany("taggable", { inverse: "tags", polymorphic: true }) taggables; } -\``` +``` It is only required that your app implement a model (or if using instantiateRecord and the schema service implement handling for that type) if your API will return entities whose type is the base type. If the base type is abstract and never directly interacted with, it is not required to implement a Model or other associated support. From 48530a033324f0d0bad8c42baa5bdeb70c5e32d1 Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Wed, 13 Apr 2022 15:50:35 -0700 Subject: [PATCH 7/7] Update 0793-polymporphic-relations-without-inheritance.md --- text/0793-polymporphic-relations-without-inheritance.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0793-polymporphic-relations-without-inheritance.md b/text/0793-polymporphic-relations-without-inheritance.md index 579febbae6..379bd08d7c 100644 --- a/text/0793-polymporphic-relations-without-inheritance.md +++ b/text/0793-polymporphic-relations-without-inheritance.md @@ -20,7 +20,7 @@ Relevant Team(s): Ember Data RFC PR: https://github.com/emberjs/rfcs/pull/793 --> -# Polymorphic relations without inheritance +# EmberData | Polymorphic Relationship Support ## Summary