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

feat!: update to latest version of clouevents sdk #402

Merged
merged 1 commit into from Dec 7, 2021

Conversation

matthewrobertson
Copy link
Member

@matthewrobertson matthewrobertson commented Dec 3, 2021

This commit updates to the latest CESDK which provides a generic interface for CloudEvents. This is an additive, non-breaking change to our registration API:

providing a template type on the registration function:

import * as functions from '@google-cloud/functions-framework';

functions.cloudEvent<string>('helloTypes', (ce) => {
  // the event data is strongly typed here
  ce.data.length;
});

providing a template type on the argument annotation:

import {CloudEvent}, * as functions from '@google-cloud/functions-framework';

functions.cloudEvent('helloTypes', (ce: CloudEvent<string>) => {
  // the event data is strongly typed here
  ce.data.length;
});

not providing a template is still allowed:

import functions from '@google-cloud/functions-framework';

functions.cloudEvent('helloTypes', (ce) => {
  // the event data has type "unknown" by default so the customer can cast to anything they want 
  const data = ce.data as string;
  data.length; // OK
});

@google-cla google-cla bot added the cla: yes label Dec 3, 2021
@matthewrobertson matthewrobertson force-pushed the udpate-cloudevent-sdk branch 4 times, most recently from c7aa8f9 to 1cf9eed Compare December 3, 2021 22:21
Copy link
Contributor

@grant grant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! This is the ideal experience I've been looking for with event types.

Had a couple comments, mainly for making sure this is tested.

test/function_wrappers.ts Outdated Show resolved Hide resolved
test/function_wrappers.ts Outdated Show resolved Hide resolved
test/function_wrappers.ts Show resolved Hide resolved
@matthewrobertson matthewrobertson force-pushed the udpate-cloudevent-sdk branch 5 times, most recently from e16b6d9 to dfdfb92 Compare December 5, 2021 16:46
Copy link
Contributor

@grant grant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good. Are we able to test with an unparameterized CloudEvent to preserve the existing test?

LGTM after requested changes.

test/function_wrappers.ts Outdated Show resolved Hide resolved
test/integration/cloud_event.ts Outdated Show resolved Hide resolved
test/function_wrappers.ts Outdated Show resolved Hide resolved
This commit updates to the latest CESDK which provides a generic interface for
CloudEvents. This is an additive, non-breaking change to our registration API
Copy link
Contributor

@grant grant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I think there might be a push that is missing.

test/function_wrappers.ts Outdated Show resolved Hide resolved
test/integration/cloud_event.ts Outdated Show resolved Hide resolved
test/function_wrappers.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@grant grant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@@ -39,7 +39,7 @@ const TEST_EXTENSIONS = {
describe('CloudEvent Function', () => {
let clock: sinon.SinonFakeTimers;

let receivedCloudEvent: functions.CloudEvent | null;
let receivedCloudEvent: functions.CloudEvent<unknown> | null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be a little funky requiring a unknown here. Question:

  • functions.CloudEvent (can we do this?)
  • functions.CloudEvent<unknown>

Comment on lines +85 to +86
| CloudEventFunction<T>
| CloudEventFunctionWithCallback<T>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible for a developer to write a CloudEventFunction without a parameterized type?

Perhaps it is not possible.

export type HandlerFunction<T = unknown> =
  | HttpFunction
  | EventFunction
  | EventFunctionWithCallback
  | CloudEventFunction<T>
  | CloudEventFunctionWithCallback<T>
  | CloudEventFunction
  | CloudEventFunctionWithCallback;

@matthewrobertson matthewrobertson changed the title feat: update to latest version of clouevents sdk feat!: update to latest version of clouevents sdk Dec 7, 2021
@matthewrobertson matthewrobertson merged commit bd36243 into master Dec 7, 2021
@matthewrobertson matthewrobertson deleted the udpate-cloudevent-sdk branch December 7, 2021 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants