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

When wait: true, serialization not applied to attributes passed to the save method. #52

Open
kbaltrinic opened this issue Jun 23, 2015 · 8 comments · May be fixed by #64
Open

When wait: true, serialization not applied to attributes passed to the save method. #52

kbaltrinic opened this issue Jun 23, 2015 · 8 comments · May be fixed by #64

Comments

@kbaltrinic
Copy link

If I have a model with a property someDate of type date, I get two different serialization results when I save with {wait: true}, depending on if I set someDate prior to invoking save, or if I pass it to the save function as follows:

model.someDate = new Date();
model.save(null, {wait: true})
// serialized to something like {"someDate": 143502072321}

model.save({someDate: new Date()}, {wait: true});
// serializes to  {"someDate": "2015-06-23T01:59:21.780Z"}

Line 67 appears to be the culprit.

Perhaps I am wrong, but something akin to the requested enhancement in ampersand-state Issue #122 seems to be needed so solve this.

@bobholt
Copy link

bobholt commented Aug 17, 2015

I can verify this. If the structure of the model is fundamentally changed in a custom serialize method, then the simple lodash.assign if {wait: true} doesn't take that into account.

The only way I can imagine this working in the current form is to create a dummy model with the new attributes, serialize that (using the same serialize method), and merge the objects for the sync request.

Example time. If this is my serialize method:

  serialize() {
    const res = this.getAttributes({props: true}, true);
    const id = res.id;

    forEach(this._children, function(value, key) {
      res[key] = this[key].serialize();
    }, this);

    forEach(this._collections, function(value, key) {
      res[key] = this[key].serialize();
    }, this);

    delete res.id;

    return {
      id,
      attributes: res,
    };
  }

It turns returns an object that looks like this:

{
  "id":"11",
  "attributes": {
    "date_end":null,
    "date_start":1344744000000
  }
}

If I do myModel.save({date_end: 1439586948920}, {wait: true});, the save attributes are added to the outer object, because of the simple lodash.assign

{
  "id":"11",
  "attributes": {
    "date_end":null,
    "date_start":1344744000000
  },
  "date_end":1439586948920
}

@pgilad
Copy link
Member

pgilad commented Aug 26, 2015

I believe this would be solved by #58

Re-open this if it didn't.

@pgilad pgilad closed this as completed Aug 26, 2015
@bobholt
Copy link

bobholt commented Sep 8, 2015

@pgilad I cannot reopen, but this issue was not resolved.

The problem is that this line:

        if (options.wait && method !== 'patch') options.attrs = assign(model.serialize(), attrs);

assigns attrs AFTER the model has been serialized. This assumes that the server expects the attributes to exist at the top level. Not all servers do.

In the wait:true situation, options.attrs should be set to a cloned copy of the model which has its attributes changed, and is THEN serialized. This avoids changing the original model until sync, while allowing the modified model to be serialized as the user directs.

I will make a PR if I get to resolving this in my project, but it's currently a lower priority.

@bobholt
Copy link

bobholt commented Sep 8, 2015

Actually started working on this, and the complexity to support all of these simultaneously quickly becomes staggering:

  • wait: true
  • patch: true
  • custom serialize that alters the form of the data

You can do 2 at a time fairly easily, but to do all 3 requires keeping track of the changed attributes (already done by state), and also being aware of way the model is serialized in order to cherry-pick those attributes out of the data.

Not impossible, but possibly ill-advised.

Which brings me to my next point: don't devise APIs that require data attributes to be in a nested object.

@bobholt
Copy link

bobholt commented Nov 6, 2015

Circling back on this so the record is here. I ended up extending AmpersandModel to support JSON-API and just got around to releasing it. I changed save within that extension to respect wait:true, patch:true, and a custom serialize method.

https://github.com/bobholt/ampersand-jsonapi-model

I think the strategy there should work within AmpersandModel. I should hopefully have some time next week to make a PR here that accomplishes the same thing.

@wraithgar wraithgar reopened this Nov 6, 2015
@wraithgar
Copy link
Contributor

Thanks for keeping with this @bobholt, sorry it didn't get reopened. Doing that now.

@bobholt
Copy link

bobholt commented Nov 6, 2015

Thanks @Wrathgar.

@bobholt
Copy link

bobholt commented Nov 13, 2015

Submitted fix for this as #64.

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 a pull request may close this issue.

4 participants