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

[workmanw]

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]

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

[workmanw]

@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.

[bmac]

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?

[workmanw]

@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 👍

[sandstrom]

@workmanw This is an awesome adjustment! ⛵

[igorT]

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

[workmanw]

@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]

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]

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]

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]

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

[xaqq]

What happened to this feature flag?

[sly7-7]

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

[xaqq]

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

Thanks.

[Exelord]

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

[allthesignals]

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]

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

[bbansalWolfPack]

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

[sandstrom]

@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]

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

[mansona]

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]

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

[mansona]

@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]

@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]

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]

@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]

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]

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.