sdk-javascript

sdk-javascript copied to clipboard

I was playing with an example on using the cloneWith method of the CloudEvent class and i noticed that the new returned(cloned) CloudEvent has the same id and time values.

So for example:

const ce = new CloudEvent({....});

ce.toJSON();   

// might be something like this:
{
  id: 12345,
  time: '1:00 pm' // i know that is not the format
}

And then if we wanted to add an extension, for instance, we could clone it into a new cloud event

const ce2 = ce.cloneWith({extenstion: 'Vaule'});

c2.toJSON()

// might be something like this:
{
  id: 12345,
  time: '1:00 pm',
  extension: 'Value'
}

I wasn't sure if that is correct or not. I can go either way on this. On one hand, it is just a clone with added features, but on the other hand, we are creating a brand new CloudEvent, so should the id and time be recreated?

@lholmquist good question. It might be worth asking in CloudEvents slack channel. My gut is to say that these values should not be cloned - especially the ID.

Not to many responses from the slack channel, other then me and you @lance thinking that those values should be updated wit ha cloneWith and @grant suggesting that we don't have this method, and just let users create a new Cloudevent themselves, which gets around the issue.

Grants suggestion: new CloudEvent({...ce.toJSON(), id: 'new-id'}), which, as he points out is how things are being done internally https://github.com/cloudevents/sdk-javascript/blob/14468980f7a79da836bd3ee8304a8a5710a206c1/src/event/cloudevent.ts#L187

We could probably remove the cloneWith, but it would have to be in a major version. i don't remember if there was a reason why we opted for hte clonewith and not just use the contructor again?

I suggest not having this method imo.

Keeping the interface as small as possible is really ideal in the long term. I deal with similar issues across consistency between implementations across 7 implementations with a similar library.

I'm fine with removing the method in 4.0.0 because it's correct that the user can do this themselves. Though frankly, I think it would be better to just deprecate it initially.

But there is one thing I think we should consider changing before that. Even though we eliminated munging the data property, there is still a get() and set() for it because we are checking for binary data, converting it to store in data_base64, and actually storing the data in #_data.

https://github.com/cloudevents/sdk-javascript/blob/8205bc96ae401099e0207bf387164fd955be7b33/src/event/cloudevent.ts#L125-L134

The side effect of this is that simply using an event as the parameter to the constructor doesn't work. You end up losing the data property.

const ce = new CloudEvent({ source: 'example', type: 'example', data: { hello: 'world' } });
const c2 = new CloudEvent({ ...ce, source: 'foo' });
console.log(c2);
CloudEvent {
  id: '5abb0646-15bf-4f4c-b431-7b2ce5206c26',
  time: '2020-11-04T21:35:37.664Z',
  type: 'example',
  source: 'foo',
  specversion: '1.0',
  datacontenttype: undefined,
  subject: undefined,
  datacontentencoding: undefined,
  dataschema: undefined,
  data_base64: undefined,
  schemaurl: undefined
}

Note that no data attribute exists. That's because there's not actually a data property on the original event. It's a getter/setter pair. This also is unfortunate because when doing console.log(ce), even if an event has data, it won't show. You need to do console.log(ce.toString()).

It's because of this that the implementation of cloneWith() requires the use of toJSON() which, if you look at the code comments, is weird since it doesn't really turn it into JSON. It returns an object. It's just what is used by JSON.stringify() allowing us to jam that data property into the output.

Anyway, this is all a long way of saying I think we should set the data_base64 property in the constructor, eliminate the get()/set() for data property, and deprecate or remove cloneWith(). The only implication is that if a user modifies the data attribute on an existing event by setting it to binary data, the data_base64 property will be either wrong (if there was binary data that got overwritten) or empty (if the original data was not binary).

Anyway, this is all a long way of saying I think we should set the data_base64 property in the constructor, eliminate the get()/set() for data property, and deprecate or remove cloneWith(). The only implication is that if a user modifies the data attribute on an existing event by setting it to binary data, the data_base64 property will be either wrong (if there was binary data that got overwritten) or empty (if the original data was not binary).

It looks like we can break this into a couple workable issues.

1. remove the getter/setter for data
2. deprecate cloneWith and remove in a later major release

as for if a user modifies the data, put a warning on the readme when we introduce the change?

This issue is stale because it has been open 30 days with no activity.