Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A CloudEvent should be readonly but provide a way to augment itself #233

Closed
lance opened this issue Jun 30, 2020 · 10 comments · Fixed by #234
Closed

A CloudEvent should be readonly but provide a way to augment itself #233

lance opened this issue Jun 30, 2020 · 10 comments · Fixed by #234
Assignees
Labels
module/lib Related to the main source code type/enhancement New feature or request version/3.x Issues related to the 3.0 release of this library

Comments

@lance
Copy link
Member

lance commented Jun 30, 2020

With recent API changes, a CloudEvent is nicely represented as a plain old JS object. For example, for most purposes, this is actually a valid CE.

const ce = {
  source: '/',
  type: 'example',
  specversion: 'v1.0',
  id: 'asd0aan3412juv'
}

However, because we want to have spec compliance checks, and provide default values (e.g. a UUID or timestamp), we have a constructor and a class. The example above can be written as:

const ce = new CloudEvent({
  source: '/',
  type: 'example',
});

Here, code in the constructor is providing default values for id and specversion.

However this introduces some tricky edge cases. For example, extensions. According to the spec, extensions must exist as siblings of the CE properties. And JS being as loosey goosey as it is, this means a user can do some things outside of the spec boundaries. For example

const ce = new CloudEvent({ source: '/', type: 'example' });
ce[extension-1] = // <- note invalid extension name
  {
    foo: 'bar' // <-- note invalid extension value
  };

A solution to this is to

  1. At the bottom of the CloudEvent constructor, call this.validate()
  2. Make the CloudEvent object read only just after validation with Object.freeze(this);
  3. Provide a "builder" of some kind that enables cloning an event with new properties/extensions, e.g. myEvent.cloneWith( { extension: 'value' }); (this could be a function on CloudEvent or a class unto itself.

Note that both #228 and #229 should be implemented and added to the validation step as a part of this.

Related: #29

@lance lance added the type/enhancement New feature or request label Jun 30, 2020
@lholmquist
Copy link
Contributor

Sounds good. Going to assign to myself

In the example, you are just talking about extensions, but i'm assuming this could go for any other value, like source or type or version, etc..?

@lholmquist lholmquist self-assigned this Jun 30, 2020
@lance
Copy link
Member Author

lance commented Jun 30, 2020

In the example, you are just talking about extensions, but i'm assuming this could go for any other value, like source or type or version, etc..?

Yes. Though it's obviously not required, since using Object.freeze() doesn't prevent the object's properties from being changed. I could, for example, change the timestamp on a "readonly" CE. freeze() just prevents adding and removing properties. We could get fancy and use TS to make the properties themselves readonly. I'm pretty sure under the covers, TS just uses the property descriptor to do that. Hmm... maybe this should be part of the change to really enforce the readonly aspect of the object.

@lholmquist
Copy link
Contributor

Something i was thinking about was trying to use a Proxy for this instead of Freezing the object and having another method that would be needed to "update" the CloudEvent.

This way, whenever a property is updated, the Proxy can "trap" the set call and run the validate method.

So doing something like this:

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

ce['addingNewExtenstion'] = 'new extenstion' 

would trigger the validate method

@lholmquist
Copy link
Contributor

Although, doing it this way(using a Proxy) would not satisfy issue #29

@lance @grant any thoughts on using the Proxy approach. I'm sort of leaning toward the Proxy approach if it works(testing things out now) instead of having a separate method to update the object

@lholmquist
Copy link
Contributor

just realized not everyone might know what this Proxy object is, https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy

@lance lance added the module/lib Related to the main source code label Jul 1, 2020
@grant
Copy link
Member

grant commented Jul 1, 2020

How about we make CloudEvents immutable like they aught to be and use Object.preventExtensions(),
and make it really easy to construct new CloudEvents?

@grant
Copy link
Member

grant commented Jul 1, 2020

I think #29 should be addressed first before this issue.

@lholmquist
Copy link
Contributor

After doing a bit of research on Proxies and a WIP PR #234 , it might be easier/more efficient to have a method an a cloudEvent that takes fields to update and merges then with the existing CloudEvents properties and then returns a new CloudEvent.

sort of what @lance suggested with myEvent.cloneWith( { extension: 'value' }); :)

While i do like the proxy approach, the implementation was starting to get a bit convoluted.

@grant
Copy link
Member

grant commented Jul 1, 2020

I would expect CloudEvents to be lightweight and immutable.

If you want to clone or slightly modify a CloudEvent, I'd propose this radical idea:

Create a CloudEvent from a CloudEvent (with additional overridden attributes):
https://github.com/cloudevents/sdk-javascript/blob/master/src/event/cloudevent.ts#L42

const modifiedCE = new CloudEvent(oldCE, { id: 'my-new-id' });

Or I could imagine a new clone(overrides) method.

@lholmquist
Copy link
Contributor

Create a CloudEvent from a CloudEvent

Yup. Doing something now. Probably sticking to the "cloneWith" method

@lance lance changed the title A CloudEvent should be readonly but provide a way to augment itsef A CloudEvent should be readonly but provide a way to augment itself Jul 2, 2020
@lholmquist lholmquist added the version/3.x Issues related to the 3.0 release of this library label Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module/lib Related to the main source code type/enhancement New feature or request version/3.x Issues related to the 3.0 release of this library
Projects
None yet
3 participants