chore(refactor): prefer interfaces over concrete classes #457
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change modifies the protocol binding interfaces such as `Binding`, `Serializer` and the like to use the `CloudEventV1` interface instead of the implementation class `CloudEvent`. This should make extending the interfaces simpler as this work has grown out of efforts around the implementation of a second transport interface, Kafka. See: cloudevents#455 This commit also includes the addition of a generic type to the `Message` interface, defaulting to `string`. There is also some minor clean up involving what is exported from the `message/http` modules. Now, instead of exporting the entire implementation, only the `HTTP` binding implementation is exported, and it is then reexported by `message`. Also, a static `CloudEvent.cloneWith()` method has been added which the instance methods now use. Signed-off-by: Lance Ball <[email protected]>
lance
added
the
chore/refactor
lance
added a commit
to lance/sdk-javascript
that referenced
this pull request
Jan 4, 2022
Also, incorporates all of the changes from cloudevents#457 Signed-off-by: Lance Ball <[email protected]>
Signed-off-by: Lance Ball <[email protected]>
lance
commented
Jan 5, 2022
| @@ -147,7 +147,7 @@ See: https://github.com/cloudevents/spec/blob/v1.0/spec.md#type-system`); | |||
| toJSON(): Record<string, unknown> { | |||
| const event = { ...this }; | |||
| event.time = new Date(this.time as string).toISOString(); | |||
| event.data = !isBinary(this.data) ? this.data : undefined; | |||
| event.data = this.#_data; | |||
There was a problem hiding this comment.
Note: It's not clear why we originally were unsetting the data property in this function if it was in binary form. But I don't think we should be doing that. Making this change had no effect on the tests. Happy to discuss what the reason is for this code as it was if anyone recalls.
Signed-off-by: Lance Ball <[email protected]>
lance
changed the title
lholmquist
approved these changes
Jan 7, 2022
grant
approved these changes
Jan 7, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change modifies the protocol binding interfaces such as
Binding,Serializerand thecloneWith()methods to use theCloudEventV1interface instead of the implementation classCloudEvent. This should make extending the interfaces simpler as using interfaces decouples Specification Extensions for relying on the concreteCloudEventclass. This work has grown out of efforts around the implementation of a second transport interface, Kafka.See: #455
In addition, this PR includes:
Messageinterface, defaulting tostringmessage/httpmodules. Now, instead of exporting the entire implementation, only theHTTPbinding implementation is exportedCloudEvent.cloneWith()method has been added which the instance methods now useI'm pretty sure all of this is fully backwards compatible as there were no changes required for the tests.
I will hold on #455 until this has landed, so that I can pull in these changes and keep 455 small-ish.
Signed-off-by: Lance Ball [email protected]