Repository navigation
lib!: change CloudEvent to use direct object notation and get/set properties - #172
Conversation
…perties
This commit makes a substantial change to the API, changing the CloudEvent class
to accept properties as an object in the constructor. For example:
```js
const CloudEvent = require('cloudevents-sdk');
// all event properties except extensions may be set in the constructor
const event = new CloudEvent({
source: 'http://my.event.source',
type: 'test-event-type'
});
// get and set all properties standard property notation
console.log(event.time); // the event timestamp
event.subject = 'my event subject';
```
Signed-off-by: Lance Ball <lball@redhat.com>
a32dbb6 to
59d5fab
Compare
Signed-off-by: Lance Ball <lball@redhat.com>
Signed-off-by: Lance Ball <lball@redhat.com>
grant
left a comment
grant
left a comment
There was a problem hiding this comment.
LGTM.
Aside: It's hard to review changes that modify HTML. Can we update HTML changes in a different branch/PR or something?
|
I realize there are a lot of changes in the HTML, however, the HTML is auto-generated, so usually I don't think it's necessary to spend time reviewing it. It's just reflecting what's in the code. Typically, I will just click the "Viewed" checkbox on the PR so they don't get in the way. Since our docs are configured to be published at https://cloudevents.github.io/sdk-javascript/ we need to have them on |
|
I would really like no autogenerated files in source control. We can think of some other way to get the docs besides this. Not sure if others agree. |
|
I think it's pretty common to use the |
|
Since it's such a big change, if you just want to see how the API is used, I added some tests that just exercise the new interface. test/cloud_event_test.js |
| // Validation Level 1 | ||
| if (!receiver.allowedContentTypes | ||
| .includes(sanityHeaders[HEADER_CONTENT_TYPE])) { | ||
| if (!receiver.allowedContentTypes.includes(sanityHeaders[HEADER_CONTENT_TYPE])) { |
There was a problem hiding this comment.
Not a blocker, but I remember something like that in other file (?) : )
if ( not_receiver_allowed_includes foo )
There was a problem hiding this comment.
Sorry missing the context here, I remember that we saw something like that in other file.
and you extracted some variables to improve the readability.
There was a problem hiding this comment.
Ahh gotcha. The difference here is that this is for a structured receiver. The PR you point to is modifying a binary event receiver where an event is sent without any content-type header. For structured receivers, the content-type must be there.
| /** | ||
| * An instance of a CloudEvent. | ||
| * An CloudEvent describes event data in common formats to provide | ||
| * interopability across services, platforms and systems. |
|
regarding some core business changes that may exist here and there I have no much idea, other than that it looks good the move from prototype to ES6 classes. LGTM after the minor changes 👍 |
Signed-off-by: Lance Ball <lball@redhat.com>
This commit makes a substantial change to the API, changing the CloudEvent class
to accept properties as an object in the constructor. For example:
This change includes a new test file to explicitly test this interface,
as well as new documentation.
Fixes: #65
Fixes: #148
Fixes: #135
Fixes: #88