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

Polymorphic relationships and native classes #6728

Closed
yratanov opened this issue Nov 12, 2019 · 8 comments · Fixed by #6767
Closed

Polymorphic relationships and native classes #6728

yratanov opened this issue Nov 12, 2019 · 8 comments · Fixed by #6767
Labels
🏷️ bug This PR primarily fixes a reported issue

Comments

@yratanov
Copy link
Contributor

yratanov commented Nov 12, 2019

Pull request with broken test

#6727

Description

I use mixins to create polymorphic associations, but I've faced the problem with using it for native classes:

export default class MyModel extends DS.Model.extend(Polymorphable) {
 ....
}

// other model
@hasMany('polymorphable', { polymorphic: true }) polymorphable; 

When I try to assign new value for it I get that MyModel is not polymorphable. When I don't use native class for MyModel it works as expected.
Is it a bug? What's the proper way of using polymorphic associations with native classes without using inheritance?

Versions

Run the following command and paste the output below:

---@0.0.0 /home/yuri/projects/---
└── ember-source@3.13.3 

---@0.0.0 /home/yuri/projects/---
└── ember-cli@3.13.1 

---@0.0.0 /home/yuri/projects/---
└── ember-data@3.13.1 

@snewcomer
Copy link
Contributor

If your models only have one level of inheritance, then export default MyModel extends Polymorphable will work with class PolyMorphable extends Model. However, if this is not possible (such as having multiple levels of inheritance), then a decorator is another option. However, you need to override the detect method. I have a colleague who has a blog post in the works on this solution...

@runspired
Copy link
Contributor

@snewcomer @yratanov we shouldn't be requiring folks to do things like override detect even if Mixins are on the way out. It's something we have supported and we should figure out a fix.

Looking at this I suspect either we need to make a tweak to how we walk the chain looking for mixins in our detection logic, or there's a bug in the native class work. Given what we do in this area is a bit messy I'm going to start by assuming it is our issue and look at the submitted test to see if I can pinpoint why.

cc @pzuraq for visibility

@pzuraq
Copy link

pzuraq commented Nov 12, 2019

I think a class decorator is probably going to be the right solution here long term. This isn't a requirement for Octane (though fixing this bug is), but it would probably mean moving away from detect since that really is entirely tied to the classic object model/mixins.

For this bug, it does appear that this works with vanilla Ember:

Screen Shot 2019-11-12 at 9 28 10 AM

@yratanov
Copy link
Contributor Author

Is it possible to change the way polymorphic associations work? For example in Rails you don't have to declare any mixins, etc, you can put any object in it, you should just set inverse in as key :
https://guides.rubyonrails.org/association_basics.html#polymorphic-associations

@Gaurav0
Copy link
Contributor

Gaurav0 commented Nov 16, 2019

I agree. There should be some way to declare polymorphic associations without mixins. Can setting an inverse just work? Or maybe a @polymorphic decorator?

@runspired
Copy link
Contributor

@Gaurav0 there is a way, you use a shared base class. The trouble with this is multi-inheritance becomes difficult-to-impossible. In the future the schema service and identifiers will allow us to not be class based at all.

@yratanov
Copy link
Contributor Author

@runspired sorry for commenting on the old issue, but is there any progress or RFC on adding better polymorphic relationships than inheritance and mixins?

@runspired
Copy link
Contributor

@yratanov there's been a ton of progress but we need an RFC. Would you be willing to drive an effort?

@runspired runspired added 🏷️ bug This PR primarily fixes a reported issue and removed Bug labels Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bug This PR primarily fixes a reported issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants