Skip to content

feat: integrate cloudevents module #160

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

Closed
wants to merge 3 commits into from
Closed

Conversation

grant
Copy link
Contributor

@grant grant commented Jul 31, 2020

Integrates the CloudEvents SDK for the Node Functions Framework. Does not change functionality.

  • Adds cloudevents module
  • Removes internal interfaces
  • Removes binary CloudEvent support for event signature. (This signature should not receive or support CloudEvents)
  • Updates tests

Potential Improvements

Production Impact

Should not have impact on GCF invoker as:

  • We don't currently use wrapCloudEventFunction in GCF
  • We don't see isBinaryCloudEvent resolving to true in wrapEventFunction
  • cloudevents bundle size is reasonable at <200kB: https://bundlephobia.com/[email protected]

CC: @lance @grayside

Signed-off-by: Grant Timmerman <[email protected]>
Signed-off-by: Grant Timmerman <[email protected]>
@grant
Copy link
Contributor Author

grant commented Jul 31, 2020

There might be some issues to address before landing this.

Current errors:

 tsc -p .

node_modules/cloudevents/dist/event/cloudevent.d.ts:15:5 - error TS1127: Invalid character.

15     #private;
       

node_modules/cloudevents/dist/event/cloudevent.d.ts:15:6 - error TS7008: Member 'private' implicitly has an 'any' type.

15     #private;
        ~~~~~~~

node_modules/cloudevents/dist/transport/index.d.ts:3:10 - error TS1005: 'from' expected.

3 export * as http from "./http/headers";
           ~~

node_modules/cloudevents/dist/transport/index.d.ts:3:10 - error TS1141: String literal expected.

3 export * as http from "./http/headers";
           ~~

node_modules/cloudevents/dist/transport/index.d.ts:3:13 - error TS1005: ';' expected.

3 export * as http from "./http/headers";
              ~~~~

node_modules/cloudevents/dist/transport/index.d.ts:3:13 - error TS2304: Cannot find name 'http'.

3 export * as http from "./http/headers";
              ~~~~

node_modules/cloudevents/dist/transport/index.d.ts:3:18 - error TS1005: ';' expected.

3 export * as http from "./http/headers";
                   ~~~~

node_modules/cloudevents/dist/transport/index.d.ts:3:18 - error TS2304: Cannot find name 'from'.

3 export * as http from "./http/headers";
                   ~~~~

node_modules/cloudevents/dist/transport/index.d.ts:3:23 - error TS1005: ';' expected.

3 export * as http from "./http/headers";         

cloudevents/sdk-javascript#272

@lance
Copy link

lance commented Jul 31, 2020

@grant consider upgrading your typescript dependency. In the cloudevents module, we are using 3.8.3 and with that version tsc does not suffer from these problems.

@grant grant mentioned this pull request Jul 31, 2020
Copy link
Contributor

@hdp617 hdp617 left a comment

Choose a reason for hiding this comment

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

LGTM. Assuming typescript is updated and the presubmit checks pass.

@grant grant mentioned this pull request Aug 3, 2020
@grant
Copy link
Contributor Author

grant commented Aug 3, 2020

Merge with master should pull the latest TS version now.

@grant
Copy link
Contributor Author

grant commented Aug 5, 2020

I think there needs to be some work with fixing the invoker for the current CE implementation before adding this module.

I've been getting into feature-creep trying to fix too many things at once. So I'll close this PR and we should continue with a smaller PR.

@grant grant closed this Aug 5, 2020
@grant grant deleted the grant_cloudevents_module branch February 27, 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.

3 participants