Happyr / message-serializer

Serialize messages for transport over the wire
MIT License
77 stars 12 forks source link

Support Timestamp on Hydration #22

Closed db306 closed 5 years ago

db306 commented 5 years ago

Hi there,

Wouldn't it make sense to have Timestamp within the Hydrator ?

public function toMessage(array $payload, int $version, int $timestamp)
{
    $foo = new Foo($payload['id'], new \DateTimeImmutable($timestamp));
}

If this makes sense i'll make a PR

aradoje commented 5 years ago

Hey. Could you elaborate more what why would we need to send the timestamp to toMessage?

db306 commented 5 years ago

We don't have access to the timestamp otherwise. A usage example would be to know the time difference between the moment the message was created and the moment this message was handled. Another usage would be perhaps a different treatment depending on the time the message was created for concurrence purposes. This timestamp can be indeed within the message/payload but if its already there... wouldn't it make sense to use it ?

aradoje commented 5 years ago

If the dispatched message "cares" about timestamps they will be part of the message anyways (hydrated into a property). From what I see it now it would add more "noise" to hydration, since not all messages need that kind of information, and also can achieve it with using of transformer/hydrator without much additional code. Right now I don't see much benefit from that. @Nyholm share your thoughts please.

db306 commented 5 years ago

I'm just giving the possibility to access the timestamp value from the toMessage function the same way we give access to the version. I don't see any complexity nor noise beyond being inclusive. Not all messages will use the version either.

db306 commented 5 years ago

Also the timestamp is not used at all anywhere else, so might as well remove it completely ?

aradoje commented 5 years ago

All messages should use version. It's the way of being able to change the payload without worrying about older messages. Like I said I don't see the benefit of having it on hand provided by hydrator over keeping it in the message.

db306 commented 5 years ago

The supportsHydrate function will make sure that we have the expected identifier and version. We already now we are supporting given version so the toMessage doesn't need the version in such case.

aradoje commented 5 years ago

Yes, if we have only version 1. If we have more than 1 version we have (at least some time) to support both. And to hydrate differently each version.

db306 commented 5 years ago

In such case it would make sense to have multiple hydrators and not a hydrator that supports multiple versions

aradoje commented 5 years ago

That is one solution. Also off topic for this issue.

db306 commented 5 years ago

I think it would make sense to give access to the whole message and not only the payload so people can do what they need. Let it be a version check or use the timestamp

db306 commented 5 years ago

That would open up many possibilities, for instance mapping messages that don't have the desired format (identifier, payload, timestamp, version). NestJS uses a diffenret wording by default, instead of 'identifier' they name it 'pattern'

Nyholm commented 5 years ago

Hm. This is an interesting topic. Thank you.

The idea is that toMessage() takes the output from getPayload() to create a message. We have the version here to make sure to version stuff.

But why add timestamp to the serialised message in the first place? That is something we dont mention in the documentation, but it is added to help the infrastructure. It allows you to setup infrastructure to not retry messages x seconds after they were originally created. It also help you log things more accurately and you have a sense of order.

I would argue not to include it in toMessage() because it is not used 100% of the times. You would always use $message and $version but $timestamp would only be used in some specific cases (as @db306 describes). I agree with @aradoje that if we need timestamp to hydrate the message we should include it in the payload.