Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EmberData | Polymorphic Relationship Support #793

Merged

Conversation

yratanov
Copy link
Contributor

@yratanov yratanov commented Feb 11, 2022

Please don't judge me, it's my first RFC :)

Some discussion here: emberjs/data#6728

rendered

@runspired runspired added the T-ember-data RFCs that impact the ember-data library label Feb 20, 2022
Copy link
Contributor

@runspired runspired left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this is headed in the right direction and mirrors what we've been discussing as the path forward for some time (simply that if the relationship definition on one model satisfies the definition on the inverse that declares itself polymorphic then that is sufficient to allow the record to satisfy the relationship). I'll leave more detailed feedback in a comment.

text/0793-polymporphic-relations-without-inheritance.md Outdated Show resolved Hide resolved
text/0793-polymporphic-relations-without-inheritance.md Outdated Show resolved Hide resolved
text/0793-polymporphic-relations-without-inheritance.md Outdated Show resolved Hide resolved
text/0793-polymporphic-relations-without-inheritance.md Outdated Show resolved Hide resolved
@runspired
Copy link
Contributor

Some prior discussion and art:

Overall this is the direction the team has wanted to take this for sometime. I think there are a few key discussion points for us all to hash out.

  1. would a much more explicit API be better here? This could include declaring intent 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. Example below with a Many:Many relationship (tags can belong to many taggables, taggables can have many tags). This can have some drawbacks in that as an API grows to have more "taggable" entities (per the example below) the config may becomes increasingly large unnecessarily. It is likely enough to the right-hand-side (Comment.tags) to just say that it satisfies the left (Tag.taggables), the left likely does not need to declare what things do satisfies it, which keeps the API flexible and allows for on-demand runtime additions to the schema.
// note how we explicitly declare what types can satisfy this relationship in "allowedTypes"
class Tag extends Model {
  @hasMany("taggable", { inverse: "tags", polymorphic: true, allowedTypes: ["post", "comment"] }) taggables;
}

// note how we explicitly set the "base type" the relationship is satisfying here
// this side is not polymorphic as only a Tag satisfies the relationship.
class Comment extends Model {
  @hasMany("tag", { inverse: "taggables", as: "taggable" }) tags;
}
  1. how does ember-data treat taggable if there is no model for the type (and thus no schema for it). Do we require that a user specify something via the schema service even if using @ember-data/model? If using @ember-data/model, do we require that a base model for that "trait" always exist just to hold that schema? Do we assume a missing inverse schema definition for a polymorphic relationship will just be created on-demand later and allow nothing to be specified?

@yratanov
Copy link
Contributor Author

Thank you @runspired! I've applied your feedback.


would a much more explicit API be better here? This could include declaring intent 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. Example below with a Many:Many relationship (tags can belong to many taggables, taggables can have many tags). This can have some drawbacks in that as an API grows to have more "taggable" entities (per the example below) the config may becomes increasingly large unnecessarily. It is likely enough to the right-hand-side (Comment.tags) to just say that it satisfies the left (Tag.taggables), the left likely does not need to declare what things do satisfies it, which keeps the API flexible and allows for on-demand runtime additions to the schema.

I really don't like having allowedTypes. Model with polymorphic relation shouldn't care what exact type is passed to it imo.

how does ember-data treat taggable if there is no model for the type (and thus no schema for it). Do we require that a user specify something via the schema service even if using @ember-data/model? If using @ember-data/model, do we require that a base model for that "trait" always exist just to hold that schema? Do we assume a missing inverse schema definition for a polymorphic relationship will just be created on-demand later and allow nothing to be specified?

Sorry, I'm not familiar with Ember Data internals, I'd assume that if inverse key is set, then we have to require it to be defined in the taggable model.

class Tag extends Model {
  @hasMany("taggable", { inverse: "tags", polymorphic: true }) taggables;
}

class Comment extends Model {
}

tag.taggable = comment; // => Error: Comment model doesn't have "tags"

// ======

class Tag extends Model {
  @hasMany("taggable", { polymorphic: true }) taggables;
}

tag.taggable = comment; // => All good

text/0793-polymporphic-relations-without-inheritance.md Outdated Show resolved Hide resolved
text/0793-polymporphic-relations-without-inheritance.md Outdated Show resolved Hide resolved
text/0793-polymporphic-relations-without-inheritance.md Outdated Show resolved Hide resolved
text/0793-polymporphic-relations-without-inheritance.md Outdated Show resolved Hide resolved
text/0793-polymporphic-relations-without-inheritance.md Outdated Show resolved Hide resolved
text/0793-polymporphic-relations-without-inheritance.md Outdated Show resolved Hide resolved
text/0793-polymporphic-relations-without-inheritance.md Outdated Show resolved Hide resolved
text/0793-polymporphic-relations-without-inheritance.md Outdated Show resolved Hide resolved
@runspired runspired changed the title Polymorphic relations without inheritance EmberData | Polymorphic Relationship Support Apr 13, 2022
@runspired
Copy link
Contributor

@yratanov I updated the RFC to fill out bits of context and design it needed as well as to update a few bits of the proposed design. To answer your above questions:

I really don't like having allowedTypes. Model with polymorphic relation shouldn't care what exact type is passed to it imo.

I think everyone is in agreement, but it's an alternative to consider. I've noted this in the alternatives.

Sorry, I'm not familiar with Ember Data internals, I'd assume that if inverse key is set, then we have to require it to be defined in the taggable model.

This wasn't what I meant. Let's say that you have a polymorphic relationship to an abstract entity (taggable), do we require that taggable ever exist on its own? or if Comment and Post satisfy Taggable can they exist without there being an explicit Taggable entity? I've answered this in the RFC updates but roughly it comes down to "you only need to implement Taggable if your API is going to return things with the type 'taggable'"

Do we need as if we have inverse and/or should we rename inverse to as.

We need both. inverse specifies the key, as specifies the abstract type.

runspired added a commit to emberjs/data that referenced this pull request Apr 16, 2022
@runspired runspired merged commit aa51fbb into emberjs:master Jul 24, 2022
@runspired runspired self-assigned this Jul 31, 2022
runspired added a commit to emberjs/data that referenced this pull request Aug 29, 2022
runspired added a commit to emberjs/data that referenced this pull request Aug 29, 2022
runspired added a commit to emberjs/data that referenced this pull request Aug 29, 2022
runspired added a commit to emberjs/data that referenced this pull request Sep 2, 2022
…7955)

* feat: explicit relationship polymorphism

* all tests passing but a few more to write

* stash

* cleanup tests and features

* add all the tests

* fix lint

* fix lint
This was referenced Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Final Comment Period T-ember-data RFCs that impact the ember-data library
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants