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

[FEATURE ds-pushpayload-return] Change pushPayload to return a value. #4110

Merged
merged 1 commit into from
Jan 27, 2016

Conversation

workmanw
Copy link

Changing store.pushPayload and serializer.pushPayload to return the "pushed value". Either a record or array of records.

You can see by the diff, this is actually a pretty trivial change and it would greatly benefit those of us using pushPayload. The only risk of breakage is if someone has overridden pushPayload in their serializer, the value may not be returned.

Addresses: #3576

@bmac
Copy link
Member

bmac commented Jan 22, 2016

@workmanw do you mind wrapping this change in a feature flag? #3979

@workmanw workmanw force-pushed the pushPayload-return branch 2 times, most recently from a0f6211 to ce4a428 Compare January 22, 2016 12:52
@workmanw
Copy link
Author

@bmac Done. I wasn't sure what to do about the documentation since it just adds a return. I can remove that for now and add it back if/when this feature flag is removed. Not sure which would be more desirable.

@workmanw workmanw changed the title Changed pushPayload to return a value. [FEATURE ds-pushpayload-return] Change pushPayload to return a value. Jan 25, 2016
@bmac
Copy link
Member

bmac commented Jan 27, 2016

Hi @workmanw we discussed this in the Ember Data meeting today and everyone present was on board with merging this behind a feature flag. Do you mind rebasing this pr so it can be merged?

…he "pushed value". Either a record or array of records.
@workmanw
Copy link
Author

@bmac Awesome. I've rebased this, and removed the API docs as we discussed on slack. Let me know if there are any further changes I can make. Thank you 👍

bmac added a commit that referenced this pull request Jan 27, 2016
[FEATURE ds-pushpayload-return] Change `pushPayload` to return a value.
@bmac bmac merged commit 2322aac into emberjs:master Jan 27, 2016
@sandstrom
Copy link
Contributor

sandstrom commented Mar 7, 2016

@workmanw This is an awesome adjustment! ⛵

@courajs courajs mentioned this pull request Mar 18, 2016
5 tasks
@@ -178,7 +179,11 @@ const JSONAPISerializer = JSONSerializer.extend({
*/
pushPayload(store, payload) {
let normalizedPayload = this._normalizeDocumentHelper(payload);
store.push(normalizedPayload);
if (isEnabled('ds-pushpayload-return')) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we do this in the store instead, and lets have the serializer return normalized json payload

Copy link
Author

@workmanw workmanw May 18, 2016

Choose a reason for hiding this comment

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

@igorT I'm not 100% sure I follow.

Currently store.pushPayload just looks up the associated serializer and invokes serializer.pushPayload. The serializers uniquely handle their normalize operations and then callback to the store (store.push).

So with all that said, I read your comment as saying, "Can we move this feature check into the store only?" In order to do that, I think you would 1) have to always just return the store.push result, or 2) create a new API mechanism for the store to ask the serializer to normalize the payload so the pushPayload` action didn't have to pass through the serializer (which I think is what maybe you were hinting at).

I'm happy to make additional changes or spend additional time continue to improve this feature. Please also tell me if I misunderstood your request. Let me know what you'd like me to do.

@workmanw
Copy link
Author

Has there been any discussion on this? I would definitely like to see it enabled and I understand if there are more factors at play, but I don't want it to be forgotten.

@srsgores
Copy link

srsgores commented Jul 27, 2016

Has this change only been made to the json-api adapter? I tried enabling this on RESTadapter, and adding the flag "ds-pushpayload-return": true doesn't work.

EDIT: reviewed the PR, and it seems it is implemented for ``RESTAdapter`. I suppose I have to enable canary mode?

@pangratz
Copy link
Member

pangratz commented Aug 1, 2016

Hey @workmanw, sorry for the long overdue response. So at the moment it wouldn't hurt if we return the pushed records from store.push within the serializer.pushPayload. As this feature outlines, the change is trivial.

But once we return the pushed records from store.pushPayload, we lock into that API. Currently we can still refactor the internals so the pushed records via pushPayload are not materialized immediately. Work on this has begun in #4272. The idea is to delay materialization of the records as long as possible, which should result in performance improvements.

I can imagine apps using WebSockets make heavy use of store.pushPayload and those would benefit automatically of those performance improvements - given that we don't return the materialized records, as we do with store.push.

I totally agree with the points you raised in #3576 (comment) regarding this being an inconsistency between store.push and store.pushPayload and also that there is a need for getting the pushed records via pushPayload.

Long story short, I've created RFC#161 to get a discussion going, in which direction we want to go so all use cases are supported. I would love to hear your input on this.

@RobbieTheWagner
Copy link
Member

Was this removed in the latest master? My pushPayload calls don't seem to return anything anymore.

@xaqq
Copy link

xaqq commented Dec 8, 2016

What happened to this feature flag?

@sly7-7
Copy link
Contributor

sly7-7 commented Dec 8, 2016

Seems like it is still used though: https://github.com/emberjs/data/search?utf8=%E2%9C%93&q=ds-pushpayload-return

@xaqq
Copy link

xaqq commented Dec 8, 2016

I was checking FEATURES.md from the master branch. Indeed the flag is described in beta's FEATURES.json.

Thanks.

@Exelord
Copy link

Exelord commented Feb 24, 2017

Hey! Is there any plan when this option be available by default?

@allthesignals
Copy link
Contributor

I'm confused - this was merged but doesn't seem to work?
Feature flag: "ds-pushpayload-return": true

let serializedPositions = this.store.pushPayload('position', positions);
serializedPositions === undefined;

https://github.com/emberjs/data/blob/master/config/features.json#L3

Thanks!

@sly7-7
Copy link
Contributor

sly7-7 commented Mar 31, 2017

@allthesignals Just to be sure, are you using a canary build of ember-data ?

@bbansalWolfPack
Copy link

is this feature added? I can not see anything returned from pushPayload method.

@sandstrom
Copy link
Contributor

@bbansalWolfPack It's not added, unfortunately.

The next steps are:

  1. The Ember Data team needs to approve this RFC: Overhaul store.push and store.pushPayload rfcs#161
  2. Someone needs to write the code/implementation of that RFC.

@Exelord
Copy link

Exelord commented Aug 27, 2018

Here is the solution for that: https://www.emberjs.com/blog/2018/04/13/ember-3-1-released.html#toc_feature-flag-code-ds-pushpayload-return-code-4-of-4
screen shot 2018-08-27 at 11 05 08

@mansona
Copy link
Member

mansona commented Aug 29, 2018

For anyone who is looking for a solution that allows you to pushPayload with different model types you can do this:

// import { singularize } from 'ember-inflector';

store.pushPayload(response);
return response.data.map(m => store.peekRecord(singularize(m.type), m.id))

assuming your response is JSON:API and has a data object with an array of models 👍

@runspired
Copy link
Contributor

@mansona I would avoid pushPayload entirely and use the pattern suggested in the release blog post that @Exelord so kindly linked :)

@mansona
Copy link
Member

mansona commented Aug 29, 2018

@runspired so that wouldn't work for me because I'm working with a mixed type array coming back across the wire. (I swear I'm not trying to be difficult 😂)

Do you see any obvious limitations to this hybrid approach:

function pushMixedPayload(store, rawPayload) {
  return rawPayload.data.map(model => {
    let ModelClass = store.modelFor(singularize(model.type));
    let serializer = store.serializerFor(singularize(model.type));

    let jsonApiPayload = serializer.normalizeResponse(store, ModelClass, assign({}, rawPayload, {
      data: model
    }), null, 'query');

    return store.push(jsonApiPayload);
  });
}

it seems to work fine for the toy(ish) example that we have right now.

@runspired
Copy link
Contributor

@mansona

  • if you are already in json-api format, there's no reason to normalize?
  • per-type serializers are really a bad pattern, per-api serializers are a better pattern
  • if you are almost to the right format and just need a few tweaks, either a helper function, single serializer, or grouping and normalizing by type prior to push would be better.

@mansona
Copy link
Member

mansona commented Aug 30, 2018

So if I pare it down to this:

function pushMixedPayload(store, rawPayload) {
  return rawPayload.data.map(model => {
    return store.push(assign({}, rawPayload, {
      data: model
    }));
  });
}

I get an error: Assertion Failed: You tried to push data with a type 'categories' but no model could be found with that name.

I'm assuming it's the serializer.normalizeResponse() that is fixing the pluralisation of types then? 🤔

As for per-type serializers, that isn't necessary. This format also works:

function pushMixedPayload(store, rawPayload) {
  let serializer = store.serializerFor('application');
  
  return rawPayload.data.map(model => {
    let ModelClass = store.modelFor(singularize(model.type));

    let jsonApiPayload = serializer.normalizeResponse(store, ModelClass, assign({}, rawPayload, {
      data: model
    }), null, 'query');

    return store.push(jsonApiPayload);
  });
}

@runspired
Copy link
Contributor

@mansona you do realize that store.push takes any json-api document, not just single-resource documents, yes?

For instance, if your only issue was needing to singularize types, this would work best:

function pushPayload(store, rawPayload) {
  if (Array.isArray(rawPayload.data)) {
    rawPayload.data.forEach(resource => resource.type = singularize(resource.type));
  } else if (rawPayload.data) {
    rawPayload.data.type = singularize(rawPayload.data.type);
  }
  
  return store.push(rawPayload);
}

as a side-note, we're working to eliminate the need to singularize and dasherize types. The only requirement will be that the format matches the modelName on disk and is consistent (e.g. if you give ember-data a singular camelCase type, then later give it the dasherized plural type that would be an error).

@mansona
Copy link
Member

mansona commented Aug 30, 2018

so this would be fine but you also need to do that all the way down the stack and singularise the includes and all the relationships in each of the objects 🤔

another naive implementation that extends your idea would be this:

function singularizeObject(data) {
  data.type = singularize(data.type)
  if(data.relationships) {
    for(let key in data.relationships) {
      singularizeObjectOrArray(data.relationships[key].data);
    }
  }
}

function singularizeObjectOrArray(item) {
  if (Array.isArray(item)) {
    item.forEach(singularizeObject);
  } else if (item) {
    singularizeObject(item);
  }
}

function pushPayload(store, rawPayload) {
  singularizeObjectOrArray(rawPayload.data);
  singularizeObjectOrArray(rawPayload.included);

  return store.push(rawPayload);
}

but again we have a problem because the API is returning dasherized attributes and I'm assuming the normaliser is the one who's job it is to fix that? 🤔 I could de-dasherize in this loop but I think we're getting into the realm of "a bit much" to be recommending 😞

thoughts?

@runspired
Copy link
Contributor

runspired commented Aug 30, 2018

You are narrowing in on what folks should be doing :)

Write a functional normalizer that works for you, and aligns your payloads to json-api with singularized, dasherized types and camelCase members.

function iterate(fn, data) {
  return Array.isArray(data) ? data.map(fn) : fn(data);
}

function normalizeType(type) {
  return singularize(dasherize(type));
}

function normalizeIdentifier(identifier) {
  identifier.type = normalizeType(identifier.type);
  return identifier;
}

export function normalizeResource(resource) {
  resource.type = normalizeType(resource.type);

  if(data.relationships) {
    for(let key in data.relationships) {
      let relationship = data.relationships[key];
      let data = relationship.data;

      if (data) {
        relationship.data = iterate(normalizeIdentifier, data);
      }
    }
  }

  return resource;
}

export function normalizeDocument(document) {
  let { data, included } = document;
  
  if (data) {
    document.data = iterate(normalizeResource, data);
  }
  
  if (included) {
    document.included = iterate(normalizeResource, included);
  }
  
  return document;
}

Then you could write a nice application serializer like this:

import EmberObject from '@ember/object';
import { normalizeDocument } from '../utils/json-api-helpers';

export default class Serializer extends EmberObject {
  normalizeResponse(_, __, almostJsonApiDocument) {
    return normalizeDocument(almostJsonApiDocument);
  },
  serialize() { ... }
}

And finally, write your push util like this for ad-hoc requests and web-socket side-channels:

import { normalizeDocument } from '../utils/json-api-helpers';

export function push(store, almostJsonApiDocument) {
  return store.push(normalizeDocument(almostJsonApiDocument));
}

Finally, once your API uses the same format as ember-data, you simply drop this normalization step in your serializer and for pushPayload you use store.push directly.

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.