Serializers

[seyz]

Serializers

[IanVS]

One problem I would have with this approach, if I understand it correctly, is it becomes less general when you have to specify a schema a priori. I am in the process of using jsonapi-serializer to override the default response of a back-end, and it needs to be completely agnostic to the models defined in any particular app. Essentially, as you have it set up, you need foreknowledge of the types and schemas. Or maybe these could be created dynamically at runtime? Then it's a matter of creating a separate JSONAPISerializer for each data type, storing it in an object, and referencing it when a response needs to be sent.

But, I guess the question I have to ask next, what does this accomplish? Is it a performance benefit? Is the cost of creating a new serializer/serialization (as currently) high enough to need to be improved?

[courajs]

My problem is basically a semantics thing. You are writing new JSONAPISerializer, but you aren't creating a serializer. You are creating a single serialized response.

[IanVS]

That I agree with. Seems strange to use a constructor function.

[SeyZ]

I agree with your suggestion. Let me think about it 👍

[SeyZ]

The alternative can be something like:

var json = new JsonApiSerializer('users', dataSet).perform({
  attributes: ['firstName', 'lastName']
});

But honestly, I'm not sure about the benefits.
Thoughts?

[SeyZ]

Feel free to reopen this issue if you want to continue this discussion.

[sarus]

I found this a bit confusing as well purely from a semantics perspective. To me it would seem more natural to use it like this:

var userSerializer = new JsonApiSerializer('users', {
  attributes: ['firstName', 'lastName']
});

var user1Json = userSerializer.serialize(data1);
var user2Json = userSerializer.serialize(data2);

[SeyZ]

I like the syntax @sarus, I agree

[sarus]

@SeyZ If you reopen this issue I'm happy to work on a PR. The big question is if you want to try to maintain backwards compatibility with the 2.x line or if this change causes a bump to 3.x.

Off the top of my head I'm not sure of a good way to make it backwards compatible though.

[sarus]

Actually for backwards compatibility here are my thoughts:

If you pass three arguments to JsonApiSerializer then we assume you are invoking the legacy behavior of returning the JSON immediately.

// Same old behavior
var json = new JsonApiSerializer('users', data, {
  attributes: ['firstName', 'lastName']
});

If you only pass two arguments (the collectionName and the options object) then we default to the new behavior which is to treat the Serializer as a reusable serializer rather than a serialization function.

var usersSerializer= new JsonApiSerializer('users', {
  attributes: ['firstName', 'lastName']
});

var json = usersSerializer.serialize(data);

The serializer.js class could look something like this (all current tests passing with this although we'd want to add tests for the new behavior)

'use strict';
var _ = require('lodash');
var SerializerUtils = require('./serializer-utils');

function JsonApiSerializer(collectionName, records, opts){
  if(arguments.length === 3){
    // legacy behavior
    this.collectionName = collectionName;
    this.opts = opts;
    return this.serialize(records);
  }else{
    // treat as a reusable serializer
    this.collectionName = collectionName;
    this.opts = records;
  }
}

JsonApiSerializer.prototype.serialize = function(records){
  var self = this;
  var payload = {};

  function getLinks(links) {
    return _.mapValues(links, function (value) {
      if (_.isFunction(value)) {
        return value(records);
      } else {
        return value;
      }
    });
  }

  function collection() {
    payload.data = [];

    records.forEach(function (record) {
      var serializerUtils = new SerializerUtils(self.collectionName, record,
          payload, self.opts);
      payload.data.push(serializerUtils.perform());
    });

    return payload;
  }

  function resource() {
    payload.data = new SerializerUtils(self.collectionName, records, payload, self.opts)
        .perform(records);

    return payload;
  }

  if (self.opts.topLevelLinks) {
    payload.links = getLinks(self.opts.topLevelLinks);
  }

  if (self.opts.meta) {
    payload.meta = self.opts.meta;
  }

  if (_.isArray(records)) {
    return collection(records);
  } else {
    return resource(records);
  }
};

module.exports = JsonApiSerializer;

[SeyZ]

Awesome idea @sarus ! Then, we will introduce warning messages when users will use the old syntax. 👍

[sarus]

How would you like the warning messages to be delivered? I took a look at util.deprecate but I don't think that will work in this case since we are not deprecating the function (just changing how it works)

[SeyZ]

Yeah, why not just console.warn?

[sarus]

sounds good I can do that. I'll have some time to work on it this week. The code change was minor but I want to update tests and documentation as well.

[SeyZ]

👍

[iamsebastian]

Any news, @sarus?

[iamsebastian]

Bump. @sarus, are you working on this issue? Otherwise, I will start implementing it by myself -- on this repo maybe.

[sarus]

Big apologies for my tardiness on this. I'll get this pull request done this week. Been very crazy at work and just had a new daughter born =) I let this slip on the priority list since the serializer has been working fine despite my desire to change around how it gets called.

[iamsebastian]

Nice. Well done. My wife forked my second son last month.
I'm looking forward to see your pull request.

[SeyZ]

Awesome guys and congrats for your forks 😄

[sarus]

@SeyZ Finished updating serializer.js and updating all the legacy tests to use the new format (legacy tests are still there as well). Do we want collection-serializer to work in the same way?

I didn't see any tests for collection-serializer or documentation as well. I assume it works as a standalone object from the serializer?