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 that are sideloaded do not materialize the relationship the first time they are pushed into the store (unless the relationship has been "primed" with a .get beforehand) #5313

Closed
kjhangiani opened this issue Jan 4, 2018 · 10 comments
Assignees
Labels
🏷️ bug This PR primarily fixes a reported issue 🏷️ test This PR primarily adds tests for a feature

Comments

@kjhangiani
Copy link

kjhangiani commented Jan 4, 2018

We have a polymorphic relationship with the following approximate structure:

post model:
comments: DS.hasMany('comment') // via mixins/commentable.js

comment model:
commentableType: DS.attr('string'),
commentableId: DS.attr('number'),
commentable: DS.belongsTo('commentable', { polymorphic: true, async: true }),

the response to store.findRecord('post', 1, { include: ['comments'] }) returns:

posts: [{ id: 1 }],
comments: [{ id: 1, commentable_type: 'Post', commentable_id: 1}, { id: 2, commentable_type: 'Post', commentable_id: 1}]

however, store.peekRecord('post', 1).get('comments') is null after this API request completes.

Interestingly, if I first do:

store.findRecord('post', 1) (without an include for comments), then call store.peekRecord('post', 1).get('comments'), this seems to "prime" the relationship so to speak, so that later, if comments are loaded from the API, either via a sideload, or directly from the comments endpoint, this relationship will correctly update.

This doesn't seem to be the case in other hasMany / belongsTo sideload situations - it seems to be due to the polymorphic nature of comments.

Here's a repro twiddle on the latest version of ember-data:

https://ember-twiddle.com/a33872a20ef7b7279c6a6566132564cb?openFiles=controllers.application.js%2C

I also tried the polymorphic PR by @runspired here: #5230 but this did not resolve the issue in our app.

Is this a bug? Is there a workaround?

@Shadowere
Copy link

Shadowere commented Jan 25, 2018

I seem to have the same problem with polymorphic relationships, if anyone's got an idea, workaround or anything.
My relationship is defined as async: true and polymorphic: true
For me, if I just use the Ember Inspector to inspect the relationship before actually trying to use it anywhere (code/template), it loads it (meaning it makes the call at that exact moment).
When I use it first directly from the code/template without inspecting it using the Ember Inspector, it doesn't even trigger a query to the server to get the data, and inspecting it after that just shows me an empty canonnical/currentState for the relationship

@kjhangiani
Copy link
Author

FYI, it looks like this is a regression from 2.14.x+, it works in 2.13.2

Here's the same twiddle, with just ember-data version downgraded to 2.13.2
https://ember-twiddle.com/d7f7a847cb0a3e93c4d0c5a8189c1967?openFiles=twiddle.json%2C

@runspired
Copy link
Contributor

This should be resolved now since #5230 landed

@bmac bmac closed this as completed Mar 16, 2018
@kjhangiani
Copy link
Author

@runspired @bmac this is actually not resolved with #5230

@runspired as per our discussion on slack, the polymorphic mixin case is still not working. Here is an updated twiddle using 2.18.2, that still exhibits the same regression:

https://ember-twiddle.com/a33872a20ef7b7279c6a6566132564cb?openFiles=twiddle.json%2C

@runspired
Copy link
Contributor

@kjhangiani ah, I forgot this was the mixin-specific issue

@runspired runspired reopened this Mar 16, 2018
@runspired
Copy link
Contributor

@kjhangiani I added it to my bucket of issues to dig in and add tests for today.

@kjhangiani
Copy link
Author

thanks @runspired - I'm interested in seeing how you write the tests when you do so - I wanted to try to write the test cases, but I got a little lost in the codebase, especially for this mixin case

@runspired runspired added the Bug label Mar 17, 2018
@runspired runspired self-assigned this Mar 17, 2018
@kjhangiani
Copy link
Author

kjhangiani commented Jul 21, 2018

just ran into this footgun again on a different polymorphic model - has there been any movement on this @runspired ?

Just tested [email protected] and the bug still exists: https://ember-twiddle.com/a33872a20ef7b7279c6a6566132564cb?openFiles=twiddle.json%2C

Our workaround is pretty hacky right now but it works

//mixins/commentable.js
export default Mixin.create({
	library: service('library'),

	comments: computed(
		'library.comments.[]',  //library.comments = store.peekAll('comment')
		function() {
			return this.get('library.comments').filterBy('commentableType', this.get('_modelName').classify()).filterBy('commentableId', this.get('intId'));
		}
	),
})

(basically we use the store to filter by type and id instead of using a hasMany)

@runspired
Copy link
Contributor

@kjhangiani I'd like to get some tests in place for it, easy fix if we have them. It's likely a non-issue on the recordData build version of ember-data, but a test would confirm either way.

@runspired
Copy link
Contributor

We should still add tests for this, but I'm going to close this as ember-data since 3.5 has not had lazy relationships and as such would not exhibit this issue.

@runspired runspired added 🏷️ bug This PR primarily fixes a reported issue 🏷️ test This PR primarily adds tests for a feature 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 🏷️ test This PR primarily adds tests for a feature
Projects
None yet
Development

No branches or pull requests

4 participants