Skip to content

Better record polymorphism - #67

Closed
igorT wants to merge 3 commits into
masterfrom
polymorphism
Closed

Better record polymorphism#67
igorT wants to merge 3 commits into
masterfrom
polymorphism

Conversation

@igorT

@igorT igorT commented Jun 19, 2015

Copy link
Copy Markdown
Member

@igorT

igorT commented Jun 19, 2015

Copy link
Copy Markdown
Member Author

It seems like people like the alternative better, I will update the RFC

@trabus

trabus commented Jun 22, 2015

Copy link
Copy Markdown

Having tried to use polymorphic models before (eventually dropping them because they weren't working the way we needed), I feel that the alternative proposal is not necessarily any simpler or clearer for developers. I also agree with the drawbacks being a big enough issue to not go with the alternative, and prefer the rfc proposal instead.

@knownasilya

Copy link
Copy Markdown
Contributor

I like the first approach, it's explicit and clear.

@Finhas

Finhas commented Jan 6, 2016

Copy link
Copy Markdown

Any update on this? I agree with @trabus and @knownasilya on this one.

In the drawbacks, you mentioned that if the polymorphic flag is not used or forgotten, you would not get data. But could a query for shape that gets

{ data: { id:1, type: 'line' }, { id:2, type: 'circle' }, { id:2, type: 'shape'}}

return

{ data: { id:2, type: 'shape'}}??

@mmoonport

Copy link
Copy Markdown

Have we given this any more thought. I like the second approach as it closely mirrors how our backend models work. It will provide consistent results with no code changes to the calls that are previously out there. And in my experience if you have a polymorphic model you are most likely calling the sub class models in finds because you can't call the parent and get all the models you'd expect back. This would greatly simplify some of our code if we could just pull the trigger on this

@btecu

btecu commented Feb 29, 2016

Copy link
Copy Markdown

I'd be glad to have either of the approaches working. Any news on this?

@ffaubry

ffaubry commented May 11, 2016

Copy link
Copy Markdown

Progress?

@leifcr

leifcr commented May 19, 2016

Copy link
Copy Markdown

Will the ids-and-type strategy in upcoming ember-data 2.6 lay parts of the groundwork for this?

Ref: http://emberjs.com/blog/2016/05/03/ember-data-2-5-released.html#toc_upcoming-features-in-ember-data-2-6-beta-1

@miguelcobain

Copy link
Copy Markdown
Contributor

@leifcr I doubt it. That feature is basically a polymorphic relationship extension. It was motivated by the fact that an id isn't enough to uniquely identify an object.

@xaqq

xaqq commented Dec 8, 2016

Copy link
Copy Markdown

Something like that is desperately needed to avoid horrible workaround.

As is, findAll / peekAll are completely broken from an OOP point of view.
Inheritance means "is-a" so it makes zero sense to not load subtypes instances when doing findAll('baseClass').

I hope support for something like this will be implemented soon.

@buritica

Copy link
Copy Markdown

hey @igorT is there something we can do to help move this forward? We hit this problem and are having issues handling some polymorphic relationships. If you give us some guidance, we could take a stab at implementing it with @sescobb27 & @NicolleJimenez

@BryanCrotaz

BryanCrotaz commented Aug 16, 2017

Copy link
Copy Markdown

I've got an interest in this as well. We built our own addon but it uses private api that's broken in ED 2.10. What's the current state?

@buritica I'm also available to help

@BryanCrotaz

Copy link
Copy Markdown

Added a proposal for automatic hierarchy discovery, including code that we use in our ED 2.8.0 addon

#244

@courajs

courajs commented Sep 26, 2017

Copy link
Copy Markdown

We could improve the "refactor hostile" drawback of the { polymorphic: true } approach.
findAll('shape') can warn in the console if shape has subclasses. Then, the warning can be dismissed by just adding { polymorphic: false }.

@BryanCrotaz

Copy link
Copy Markdown

@courajs sounds good. Default should definitely be polymorphic. Should be able to turn the warning off, otherwise it will flood a real polymorphic app.

@BryanCrotaz

Copy link
Copy Markdown

3 years since the original issue is there any progress? I'm using an addon to fake some of it but it uses private API so is tied to 2.10. 2.10 stops me upgrading ember to use glimmer as pushPayload is broken with later ember.

I'm.
Stuffed.
Would love to help but I have no idea what progress has been made or what help is needed

@JackLaBarba

Copy link
Copy Markdown

I'm using an addon to fake some of it

@BryanCrotaz, do you think your addon might make a good starting point to implement this RFC? If it's available on github, I'd love to take a look. My team has been maintaining some custom adapter/serializer code in our app to essentially handle what this RFC proposes but I'd love to use and contribute to a community solution instead.

@knownasilya

Copy link
Copy Markdown
Contributor

Not sure if https://github.com/EmberMap/ember-data-storefront would be a good place for this, worth checking there too.

@runspired

Copy link
Copy Markdown
Contributor

I suspect now that Ember has true class inheritance that this is something we could support more easily and would be nice to revisit.

@igorT igorT self-assigned this Jun 5, 2019
@runspired

Copy link
Copy Markdown
Contributor

I think we have a much simpler path to polymorphism for everything except findAll

  • for query and queryRecord and findRecord we return whatever was in the primary data membership of the api response, we blindly assume any records in there are valid records, since the data key signifies that to us.
  • for findAll (which returns peekAll), peekRecord and peekAll we continue to return only the narrowed sub-type. For these methods polymorphism is easily solved for in user space by joining two arrays or joining multiple peeks spanning the desired types.
  • we can however also add a new API for peek! because of the work on identifiers, apps have a public API available to them by which to create secondary indeces for records. This can include polymorphic lookup indeces and emner-data's cache already keeps track of what polymorphic types exist for a given identifier: https://github.com/emberjs/data/blob/master/packages/-ember-data/tests/integration/identifiers/polymorphic-scenarios-test.ts#L96. We could likely introduce a new peek API that returns the identifiers for any records of a given base-type.

@sly7-7

sly7-7 commented May 25, 2022

Copy link
Copy Markdown

@runspired Not sure my use case is related, but I would something like 'lazy load' records coming from an async polymorphic hasMany, one at a time, to be able to put placeholders while each record is loading.
For now, I'm using a private api from hasManyReference,
like the ids() function (https://github.com/emberjs/data/blob/master/packages/store/addon/-private/system/references/has-many.ts#L204), but using the whole identifiers, and then I can store.findRecord(identifer.type, identifier.id).
Do you think something like hasManyReference#identifiers() could make sense ? (or adding a parameter to ids() to return the full identifier)

@wagenet

wagenet commented Jul 25, 2022

Copy link
Copy Markdown
Member

@runspired do you think this RFC can be revived? or does it make more sense to write a new one?

@runspired

Copy link
Copy Markdown
Contributor

@wagenet probably best as a new RFC

@wagenet

wagenet commented Jul 25, 2022

Copy link
Copy Markdown
Member

@runspired I'll go ahead and close. If you need a tracking issue will you be able to take care of that?

@wagenet wagenet closed this Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-ember-data RFCs that impact the ember-data library

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.