Move JSON.stringify from PubSub to DynamoDBEventStore and make it optional

[ebarault]

Hi @michalkvasnicak,

This is more a question at this stage, but after digging more into your implementation, I wonder what is the rationale for stringifying the message payload in the PubSub publish method ?

  publish = async (eventName: string, payload: any) => {
    if (typeof eventName !== 'string' || eventName === '') {
      throw new Error('Event name must be nonempty string');
    }

    await this.eventStore.publish({
      payload: JSON.stringify(payload),
      event: eventName,
    });
  };

On the contrary, the SubscriptionEvent type does not expect the String type:

export interface ISubscriptionEvent {
  event: string;
  payload: any;
}

Is this in an effort to ease EventStore backend implementations ?

Thank you

cc: @SuperHamouch11

[michalkvasnicak]

@ebarault sorry for the late response. That's actually a good question. I'm not quite sure at the moment why I used JSON.stringify. I think it's from the legacy 0.x version which used DynamoDB mainly and I wanted to make sure that userland code is trying to store only JSON serializable values to DynamoDB.

Does it cause problems for you?

[ebarault]

@michalkvasnicak Well actually in particular in the contexte of DyanamoDB i don't see the reason why to force the payload to be a string, as DynamoDB would accept any arbitrary form of JSON object.

We have a situation where the writer of the events in the DynamoDB event table is totally out of scope of this library, as it cannot use a graphql mutation to write the events. So we have to fallback to the writing the raw data in DynamoDB, and it clutters a bit the code to have to JSON.stringify the payload.

Also it make the payload unreadable in the DyanamoDB explorer, which is bad and when debugging.

It would be great if you could relax this constrain.
What do you think?

[michalkvasnicak]

@ebarault yes I you're right. The problem is backward compatibility. Maybe we could remove JSON.serialize from PubSub and use it directly in DynamoDBEventStore and add an option to disable it completely so it won't break existing apps after upgrade.

[ebarault]

@michalkvasnicak yes, that sounds like a reasonable way forward to offer more flexibility while keeping backward compatibility

[michalkvasnicak]

Ok so I found a little time to do something about this. It seems it doesn't matter if there is JSON.stringify because if you are storing events to DynamoDB from some other place and you aren't using PubSub at all, then you can store an object, see https://github.com/michalkvasnicak/aws-lambda-graphql/blob/master/packages/aws-lambda-graphql/src/ArrayPubSub.ts#L35. But I made a change to PubSub that it takes an optional option serializeEventPayload that can be set to false if you don't want to use JSON.serialize in PubSub