-
Notifications
You must be signed in to change notification settings - Fork 69
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
fix: introduce CloudEventV1 and CloudEventV03 interfaces #194
Conversation
Additionaly, this commit changes all of the user facing API to be `.ts` instead of `.js` files. The existing documentation in `./docs` was removed. It should be replaced with generated HTML from tsdocs, pending some other method of publishing API documentation. That will come as a separate, docs-only PR. Fixes: https://github.com/cloudevents/skd-javascript/issues/188 Signed-off-by: Lance Ball <lball@redhat.com>
Signed-off-by: Lance Ball <lball@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. 1 comment.
@grant what do you think about the tradeoffs re: auto completion? As this PR stands now, the only way to create a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TLDR: Create a different interface for the unique constructor required params.
Maybe we can use TypeScript unions to not duplicate TypeScript interfaces.
Example: https://codingblast.com/typescript-union-types-type-guards-type-aliases/
interface CloudEventV1Constructor { ... };
interface CloudEventV1 { ... } | CloudEventV1Constructor;
The easiest solution would be to indicate that
id
andspecversion
are optional in the interfaces. But I'm not sure I like doing that, since a cloud event must have these attributes. They are optional in the ctor for theCloudEvent
class because they are either generated or have a default value if not provided.
I don't think we should make required CE fields optional if they aren't optional.
For the constructor, we can make them optional by accepting a different interface.
For generated fields like specversion, we could have a constructor that accepts JSON objects that accepts either interface.
One option here would be to make the
CloudEvent
constructor acceptCloudEventV1
,CloudEventV03
, orany
.
How about CloudEventV1Constructor
| CloudEventV03Constructor
?
In general, it is bad practice to use type any
. We should try to keep the usage of this type to a minimum and eventually add a no-any
rule to our tsconfig. We do not want to accept a new CloudEvent(1)
for example.
Signed-off-by: Lance Ball <lball@redhat.com>
This commit extracts all of the attributes from a `CloudEventVX` that are not generated by the constructor (id and specversion) into their own `CloudEventVXAttributes` interface which the `CloudEventVX` interface now extends. This allows TS devs to optionally provide `id` and `specversion` with proper autocompletion. Additionally, I have added a union type, `Event` in `cloudevent.ts` which represents any of `CloudEventV1`, `CloudEventv03`, `CloudEventV1Attributes` and `CloudEventV03Attributes`. Signed-off-by: Lance Ball <lball@redhat.com>
Signed-off-by: Lance Ball <lball@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should go with this. There are some nits like no-any
but they probably require a bit of work to fix.
Ref: microsoft/TypeScript#1897
const headers = {}; | ||
let headerMap; | ||
if (version === SPEC_V1) { | ||
headerMap = EmitterV1; | ||
} else if (version === SPEC_V03) { | ||
headerMap = EmitterV03; | ||
} | ||
headerMap.forEach((parser, getterName) => { | ||
headerMap.forEach((parser: any, getterName: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the type of parser
? We probably want to make the Base64Parser
and JSONParser
extend the same parser class.
We probably don't need classes though, since these parsers should really be methods and simplified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to open a new issue/pr for this. There are a bunch of changes that still could be done to the codebase to improve it overall - nits like no-any
as you mention above as well as some of these bigger style/architecture concerns.
Nice job applying the interface union suggestion @lance. 👍 |
Signed-off-by: Lance Ball <lball@redhat.com>
Signed-off-by: Lance Ball <lball@redhat.com>
OK - this is landing! I will cut a 2.0.1 release either over the weekend or on Monday. |
This commit introduces
CloudEventV1
andCloudEventV03
TypeScript interfaces. Additionally, it changes all of the user facing API to be.ts
instead of.js
files. Those are:lib/cloudevent.ts
lib/bindings/http/http_receiver.ts
lib/bindings/http/http_emitter.ts
lib/bindings/http/constants.ts
Usage in JavaScript
The pure JS API has not changed. You still create a
CloudEvent
like this.Usage in TypeScript
This change resolves the auto-completion issue with raw JavaScript objects through the use of the new interfaces.
Example:
Documentation
The existing documentation in
./docs
was removed and should replaced with generatedHTML from tsdocs, pending some other method of publishing API documentation. To keep
this commit relatively small and focused on the code, that will come as a separate docs-only PR.
Fixes: #184
Fixes: #188
Signed-off-by: Lance Ball lball@redhat.com