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

Provide an extension point in JSONSerializer to determine which hasMany relationships should be serialized #2494

Closed
rsutphin opened this issue Nov 20, 2014 · 13 comments

Comments

@rsutphin
Copy link

A frequently brought up issue (#2068, #1678, 7f752ad, to name a few) is the fact that JSONSerializer#serializeHasMany does not serialize manyToOne (i.e., bidirectional) hasMany relationships. It is clear from the discussion elsewhere that this is a considered behavior and won't be changed.

The recommended alternative is to use EmbeddedRecordsMixin in a custom serializer for each model to indicate which hasManys should be serialized. If you only want some bidrectional hasManys to be treated this way this seems like a fine solution.

However, if you want to serialize all hasMany relationships[1], it adds complexity and cognitive load — every time you add a new hasMany relationship, you have to remember add it to a serializer somewhere else also. So practically, the solution that is used (e.g., in ember-localstorage-adapter, ember-indexeddb-adapter, in my own app using ember-pouch) is to copy serializeHasMany from DS.JSONSerializer into the application's serializer and add manyToOne to the list of relationships to be serialized.

This is less than ideal for all the reasons that copying and pasting code is always less than ideal. It would be great if JSONSerializer could provide an extension point that a customized serializer could use to indicate which hasMany associations should be serialized by default, without requiring that the rest of the implementation be copied also.

I'd be happy to write a PR for this. Question: are the values returned by determineRelationshipType considered part of the public API for ember-data and therefore unlikely to change?

[1]: One reason you'd want to do this that I'm aware of is if your read and write formats are the same. This happens for sure when you are writing to a local document store, like PouchDB or IndexedDB.

@broerse
Copy link

broerse commented Dec 13, 2014

In ember-data the hasMany array should not be a normal array but the end result of a query over the belongsTo items. (just as on the backend)
If a new extra id is added remote to the remote hasMany array and comes in via the adapter it should only be added if it matches a belongsTo item.
This means that you should never listen to the incoming hasMany array and only set/remove id's in the hasMany array if belongsTo item are changed, added or deleted.
So there is no need for the above if ember-data fixes this. Even if an empty hasMany array comes in via the adapter it should be ignored.

@igorT
Copy link
Member

igorT commented Jun 9, 2015

cc @sly7-7 I think you are handling this

@sly7-7
Copy link
Contributor

sly7-7 commented Jun 9, 2015

If we consider adding {serialize: true} to the attrs hash to be a solution, then yes the PR I'm working on should handle this.

@broerse
Copy link

broerse commented Jun 9, 2015

@sly7-7 Looking forward to it.

@sly7-7
Copy link
Contributor

sly7-7 commented Jun 9, 2015

@broerse you could take a look at #3214, specifically the tests. Let us know if this fits your need

@broerse
Copy link

broerse commented Jun 9, 2015

@sly7-7 I will look at it tomorrow night. Do you @fsmanuel @rsutphin @OleRoel real experts have time to take a look.

@broerse
Copy link

broerse commented Jun 10, 2015

I am still a beginner and new to this kind of testing. I updated my old, not working with hashMany, project:
https://github.com/broerse/ember-select-test
and have to work on some deprecations but how do I get the new patch in bower? It fails now.

When {serialize: true} works pages should be filled if you select "test4":
http://martinic.iriscouch.com/_utils/document.html?selecttest/modelname_2_9D69343F-6FF0-BB4F-AF87-82074E9CF99B

@broerse
Copy link

broerse commented Jun 13, 2015

@sly7-7 I have converted my old test https://github.com/broerse/ember-select-test

but get Error while processing route: pages.index Cannot read property 'has' of undefined TypeError: Cannot read property 'has' of undefined when I switch to canary. If you have time please take a look. When everything works the hasmany array should be filled in iriscouch and local.

@broerse
Copy link

broerse commented Jun 18, 2015

More about this error: #3373

@wecc
Copy link
Contributor

wecc commented Sep 16, 2015

The public way to do this is to provide { serialize: true } in the attrs hash in your serializer, like you're describing.

The non-public way would be to override _shouldSerializeHasMany and return true.

Do we want a public way to override this on a broader level? @igorT @bmac

@igorT
Copy link
Member

igorT commented Oct 13, 2015

I would say yes. Wanna get someone to make a PR and feature flag it?

@bmac
Copy link
Member

bmac commented Oct 13, 2015

For clarification @igorT is suggesting we make _shouldSerializeHasMany a public method (drop the underscore) and allow users to over rider it.

@wecc
Copy link
Contributor

wecc commented Oct 21, 2016

Closing this since shouldSerializeHasMany is now public.

@wecc wecc closed this as completed Oct 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants