Add Pub/Sub support + HTTPS testing #925

inlined · 2021-07-30T20:55:30Z

While adding Pub/Sub support I noticed that there were now v2/ tests.
Added those tests + fixes that they uncovered.

Also fixed issue where 'firebase-functions/logger' wasn't working as
an import in my test project.

While adding Pub/Sub support I noticed that there were now v2/ tests. Added those tests + fixes that they uncovered. Also fixed issue where 'firebase-functions/logger' wasn't working as an import in my test project.

mbleigh · 2021-07-30T21:45:44Z

src/v2/base.ts

@@ -22,6 +22,9 @@

 /** @internal */


If this is going to be a package export, perhaps we should call it core.ts instead of base.ts? Base to me seems like a class that will be extended whereas this is more shared interfaces.

mbleigh · 2021-07-30T21:47:18Z

src/v2/base.ts

+
+  /** The resource which published this event. */
+  source: string;
+


Shouldn't there also be a subject?: string here? Also, have you thought about how we want to handle a replacement for context.params in a v2 world since it won't be in the payload and isn't a part of CloudEvent?

RE subject, yes. I was just documenting the fields I saw in prod and subject was missing for P/S events.
RE params, why not event.params?

mbleigh · 2021-07-30T21:49:16Z

src/v2/providers/https.ts

@@ -44,12 +44,16 @@ export type HttpsHandler = (
  request: Request,
  response: express.Response
 ) => void | Promise<void>;
-export type CallableHandler = (
-  data: any,
+export type CallableHandler<T> = (


We could make this CallableHandler<T,U> and let both the request and response types be specified.

I was going to do that in a followup conversation per Chat, but can do it now.

mbleigh · 2021-07-30T21:50:45Z

src/v2/providers/pubsub.ts

+ *
+ * @param data Payload of a Pub/Sub message.
+ */
+export class Message {


Should this be generic as well, allowing folks to specify onMessagePublished<T>({data: {json: T}})?

I'm worried that this will affect readability more than it improves ergonomics, but we can try it out.

mbleigh · 2021-07-30T21:52:54Z

src/v2/providers/pubsub.ts

+  constructor(data: any) {
+    this.messageId = data.messageId;
+    this.data = data.data;
+    (this.attributes = data.attributes || {}), (this._json = data.json);


This line is super weird, what's going on here? I would expect the comma to be a syntax error...

My fingers may have run faster than my brain and code complete probably had a hand. In this case, a comma is the same as a semicolon though the parens are necessary due to order of operations. Same with C++.

mbleigh · 2021-07-30T21:59:38Z

src/v2/providers/pubsub.ts

+  }
+
+  const func = (raw: CloudEvent<unknown>) => {
+    const messagePublisheData = raw.data as {


Missing a d at the end of "Published"

mbleigh · 2021-07-30T21:59:55Z

src/v2/providers/pubsub.ts

+
+  func.run = callback;
+
+  func.__trigger = 'silencing transpiler';


Added comments

mbleigh · 2021-07-30T22:03:13Z

spec/v2/providers/pubsub.spec.ts

+
+  it('should create a complex trigger with appropraite values', () => {
+    const result = pubsub.onMessagePublished(
+      {


I wonder if we should extract out a common "kitchen sink" trigger that can be used across tests...

import { FULL_OPTIONS, FULL_TRIGGER } from "./helpers"; pubsub.onMessagePublished({ ...FULL_OPTIONS, topic: 'topic' }); expect(result.__trigger).to.deep.equal({ ...FULL_TRIGGER, eventTrigger: EVENT_TRIGGER });

I can buy it

mbleigh · 2021-07-30T22:04:25Z

src/v2/providers/pubsub.ts

+
+export function onMessagePublished(
+  topicOrOptions: string | PubSubOptions,
+  callback: (event: CloudEvent<MessagePublishedData>) => any | Promise<any>


I'd prefer "handler" to "callback" I think

mbleigh · 2021-07-30T22:04:52Z

src/v2/providers/https.ts

  context: CallableContext
 ) => any | Promise<any>;

 export type HttpsFunction = HttpsHandler & { __trigger: unknown };
+export interface CallableFunction<T> extends HttpsHandler {
+  __trigger: unknown;
+  run(data: T, context: CallableContext): any | Promise<any>;


I probably knew this at one point, but what's with .run?

inlined

Finally someone willing to stand up to me in code reviews! 😄

inlined · 2021-07-30T22:25:37Z

spec/v2/providers/pubsub.spec.ts

+
+  it('should create a complex trigger with appropraite values', () => {
+    const result = pubsub.onMessagePublished(
+      {


I can buy it

inlined · 2021-07-30T22:32:06Z

src/v2/base.ts

@@ -22,6 +22,9 @@

 /** @internal */


inlined · 2021-07-30T22:32:59Z

src/v2/base.ts

+
+  /** The resource which published this event. */
+  source: string;
+


RE subject, yes. I was just documenting the fields I saw in prod and subject was missing for P/S events.
RE params, why not event.params?

inlined · 2021-07-30T22:33:25Z

src/v2/providers/https.ts

@@ -44,12 +44,16 @@ export type HttpsHandler = (
  request: Request,
  response: express.Response
 ) => void | Promise<void>;
-export type CallableHandler = (
-  data: any,
+export type CallableHandler<T> = (


I was going to do that in a followup conversation per Chat, but can do it now.

inlined · 2021-07-30T22:40:43Z

src/v2/providers/pubsub.ts

+ *
+ * @param data Payload of a Pub/Sub message.
+ */
+export class Message {


I'm worried that this will affect readability more than it improves ergonomics, but we can try it out.

inlined · 2021-07-30T22:47:32Z

src/v2/providers/pubsub.ts

+   */
+  get json(): any {
+    if (typeof this._json === 'undefined') {
+      this._json = JSON.parse(


inlined · 2021-07-30T22:47:56Z

src/v2/providers/pubsub.ts

+
+export function onMessagePublished(
+  topicOrOptions: string | PubSubOptions,
+  callback: (event: CloudEvent<MessagePublishedData>) => any | Promise<any>


inlined · 2021-07-30T22:50:05Z

src/v2/providers/pubsub.ts

+  } else {
+    topic = topicOrOptions.topic;
+    opts = { ...topicOrOptions };
+    delete (opts as any).topic;


I could equally easily just say opts = topicOrOptions. optionsToTriggerAnnotations will skip over anything that's unknown. I definitely don't want that function altering its parameters though. I just thought here that clone = {... original }; delete clone.unwanted was a fine way of saying _.omit(original, "unwanted") and removing the unwanted field is just defensive programming.

inlined · 2021-07-30T22:51:11Z

src/v2/providers/pubsub.ts

+  }
+
+  const func = (raw: CloudEvent<unknown>) => {
+    const messagePublisheData = raw.data as {


inlined · 2021-07-30T22:51:17Z

src/v2/providers/pubsub.ts

+
+  func.run = callback;
+
+  func.__trigger = 'silencing transpiler';


Added comments

Add Pub/Sub support + HTTPS testing

8b7d427

While adding Pub/Sub support I noticed that there were now v2/ tests. Added those tests + fixes that they uncovered. Also fixed issue where 'firebase-functions/logger' wasn't working as an import in my test project.

inlined requested a review from mbleigh July 30, 2021 20:55

google-cla bot added the cla: yes label Jul 30, 2021

mbleigh reviewed Jul 30, 2021

View reviewed changes

PR feedback

8226a89

inlined commented Jul 30, 2021

View reviewed changes

mbleigh approved these changes Jul 31, 2021

View reviewed changes

inlined merged commit 3d70007 into master Aug 2, 2021

inlined deleted the inlined.pubsub branch August 11, 2021 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Pub/Sub support + HTTPS testing #925

Add Pub/Sub support + HTTPS testing #925

inlined commented Jul 30, 2021

mbleigh Jul 30, 2021

inlined Jul 30, 2021

mbleigh Jul 30, 2021

inlined Jul 30, 2021

mbleigh Jul 30, 2021

inlined Jul 30, 2021

mbleigh Jul 30, 2021

inlined Jul 30, 2021

mbleigh Jul 30, 2021

inlined Jul 30, 2021

mbleigh Jul 30, 2021

inlined Jul 30, 2021

mbleigh Jul 30, 2021

inlined Jul 30, 2021

mbleigh Jul 30, 2021

inlined Jul 30, 2021

mbleigh Jul 30, 2021

inlined Jul 30, 2021

mbleigh Jul 30, 2021

inlined left a comment

inlined Jul 30, 2021

inlined Jul 30, 2021

inlined Jul 30, 2021

inlined Jul 30, 2021

inlined Jul 30, 2021

inlined Jul 30, 2021

inlined Jul 30, 2021

inlined Jul 30, 2021

inlined Jul 30, 2021

inlined Jul 30, 2021


		/** The resource which published this event. */
		source: string;


		func.run = callback;

		func.__trigger = 'silencing transpiler';

Add Pub/Sub support + HTTPS testing #925

Add Pub/Sub support + HTTPS testing #925

Conversation

inlined commented Jul 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inlined left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment