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

Add override so serialiser saves hasMany #111

Closed

Conversation

backspace
Copy link
Collaborator

As suggested here by @fsmanuel.

What I missed in my previous pull request was that the dummy application wasn’t using the PouchDB serialiser!

@@ -0,0 +1,3 @@
import { Serializer } from 'ember-pouch/index';
Copy link
Collaborator

Choose a reason for hiding this comment

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

you don't need index:
import { Serializer } from 'ember-pouch';

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, thanks. I emulated this line. Changed now.

@fsmanuel
Copy link
Collaborator

The failing test is related to the change:

Integration | Adapter | Basic CRUD Ops: update an existing record

Error: Assertion Failed: A (subclass of DS.Model) record was pushed into the store with the value of ingredients being 'undefined', but ingredients is a hasMany relationship so the value must be an array. You should probably check your data payload or serializer.

I don't know if it's a bug in ember-data 1.13.

@backspace backspace force-pushed the shouldSerializeHasMany branch from 575459f to ff1643c Compare January 31, 2016 22:23
@backspace
Copy link
Collaborator Author

@fsmanuel, I was able to get the tests passing on Ember 1.13 by adding this override of serializeHasMany:

  serializeHasMany(snapshot, json, relationship) {
    this._super.apply(this, arguments);

    const key = relationship.key;

    if (!json[key]) {
      json[key] = [];
    }
  }

If the Ember Data implementation of serializeHasMany would result in an undefined attribute, this replaces it with an empty array. It seems kind of weird to me, but it works… do you think I should add this to the PR?

@fsmanuel
Copy link
Collaborator

fsmanuel commented Feb 1, 2016

@backspace LGTM!

@backspace backspace force-pushed the shouldSerializeHasMany branch from ff1643c to 8630e55 Compare February 1, 2016 13:17
@nolanlawson
Copy link
Member

Seems to be failing only in Ember Data 1.13:

A (subclass of DS.Model) record was pushed into the store with the value of ingredients being 'undefined', but ingredients is a hasMany relationship so the value must be an array. You should probably check your data payload or serializer.

Do we want to drop support for 1.13 or is this fixable?

@backspace
Copy link
Collaborator Author

@nolanlawson: I just pushed that override for serializeHasMany that fixes it on 1.13. It’s a bit awkward but seems harmless.

@nolanlawson
Copy link
Member

LGTM! Thanks for the fix; passing in Travis now.

@nolanlawson
Copy link
Member

3cb2231

@nolanlawson nolanlawson closed this Feb 1, 2016
@mattmarcum mattmarcum mentioned this pull request Mar 19, 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

Successfully merging this pull request may close these issues.

3 participants