Generic interface for provider enhancement #1020

colerogers · 2022-01-19T22:43:55Z

Change for the generic interface. Exposes onAlertPublished from the v2/alerts namespace

Ex ~

import { onAlertPublished } from "firebase-functions/v2/alerts";

export const func1 = onAlertPublished("crashlytics.newFatalIssue",(event) => { … });

export const func2 = onAlertPublished(
  { alertType: "crashlytics.newFatalIssue" },
  (event) => { … }
);

export const func3 = onAlertPublished(
  { 
    alertType: "crashlytics.newFatalIssue",
    appId: "123456789"
  },
  (event) => { … }
);

taeold

Couple of suggestions and changes.

spec/v2/providers/alerts/alerts.spec.ts

spec/v2/providers/helpers.ts

src/v2/core.ts

src/v2/providers/alerts/alerts.ts

taeold · 2022-01-20T22:11:01Z

src/v2/providers/alerts/alerts.ts

+  | 'appDistribution.newTesterIosDevice'
+  | string;
+
+/** Options */


nit* I found some of these comments kinda unhelpful - e.g. I can tell from the interface name that this is an option. Can we expand on the documentations or get rid of them?

I had those comments to break up the file into sections, but since we don't really do that anywhere else, I've made them a bit more descriptive

taeold · 2022-01-20T22:15:06Z

src/v2/providers/alerts/alerts.ts

+}
+
+/** Cloud Event Type */
+interface WithAlertTypeAndApp {


I think type names should be a noun and not adjectives, so maybe something like AlertEventExt is more appropriate.

(I think the & type intersection encompasses the with you are trying to capture here).

I notice that this name was approved in the API proposal. I should've objected then - feel free to ignore.

src/v2/providers/alerts/alerts.ts

taeold · 2022-01-20T22:18:56Z

spec/v2/providers/alerts/alerts.spec.ts

+    });
+  });
+
+  describe('defineEndpoint', () => {


I kinda liked our way of testing high-level, external API (e.g. onAlertPublished) vs internal helper function like defineEndpoint. So we could just add a test case to check __endpoint prop is approriately set on each external API instead. WDYT?

I think i now understand the purpose of defineEndpoint - it's meant to be used across all alerts API. This feels fine then. A minor suggestion for renaming to addEndpointAnnotation to make it a little more obvious that calliing this fn has side-effect.

I ended up changing the function to return the endpoint object (from your comment below) and re-named it to getEndpointAnnotation. How do you feel about that?

taeold · 2022-01-20T22:22:25Z

src/v2/providers/alerts/alerts.ts

+  alertType: string,
+  appId?: string
+): void {
+  Object.defineProperty(func, '__endpoint', {


I don't think this has to be a dynamic property. We did this in storage trigger only because we needed to get bucket from FIREBASE_CONFIG, but that's not the case here. See https.ts for an example of static __endpoint definition!

Makes sense, changed to static definition

taeold · 2022-01-20T22:23:14Z

src/v2/providers/alerts/alerts.ts

+  // that __endpoint doesn't exist. We can either cast to any and lose all type safety
+  // or we can just assign a meaningless value before calling defineProperty.
+  func.__trigger = 'silence the transpiler';
+  func.__endpoint = {} as ManifestEndpoint;


Would prefer

func.__endpoint = defineEndpoint(...)

since I think we should shy away from type assertion if it's unnecessary.

Just inlining the function implementation here could also work fine?

Re-named and changed to

func.__endpoint = getEndpointAnnotation(...);

spec/v2/providers/alerts/alerts.spec.ts

taeold

lgtm, one minor comment on test

spec/v2/providers/alerts/alerts.spec.ts

src/v2/providers/alerts/alerts.ts

inlined · 2022-01-27T08:02:43Z

src/v2/providers/alerts/alerts.ts

+      eventFilters: {
+        alertType,
+      },
+      retry: false,


Do we always hard code retry: false? We should be supporting booleans here now that GCF gen 2 supports retry (though I actually have a thread with them about changing this from a boolean to a more featured struct)

Replaced this hard coded value with opts.retry in the latest commit

inlined · 2022-01-27T08:04:10Z

src/v2/providers/alerts/alerts.ts

+ */
+export function getOptsAndAlertTypeAndApp(
+  alertTypeOrOpts: AlertType | FirebaseAlertOptions
+): [options.EventHandlerOptions, string, string | undefined] {


Returning a tuple is pythonic. Returning an object would have probably been more idiomatic JS. Regardless, this is probably easier to read so LGTM.

* adding in app distro changes * removing comments & addings package refs * jsdoc comments * fix comments * adding periods to doc strings * fix wording

* adding in billing changes * removing comments & adding package refs * jsdoc comments * addressing pr comments * change handler doc string * changing to BillingEventHandler type * remove BillingEventHandler type

* adding in crashlytics changes * comments & adding package refs * address comments and make doc strings better * add opts.retry to event trigger

* breaking out general interface * cleaning up exports * removing comments & references to specific interfaces * format * jsdoc comments * address pr comments * added param comment * addressing comments * Specific interface for provider enhancement (1/3) (#1021) * adding in app distro changes * removing comments & addings package refs * jsdoc comments * fix comments * adding periods to doc strings * fix wording * adding import/export statement * Specific interface for provider enhancement (2/3) (#1022) * adding in billing changes * removing comments & adding package refs * jsdoc comments * addressing pr comments * change handler doc string * changing to BillingEventHandler type * remove BillingEventHandler type * Specific interface for provider enhancement (3/3) (#1023) * adding in crashlytics changes * comments & adding package refs * address comments and make doc strings better * add opts.retry to event trigger

colerogers added 2 commits January 19, 2022 14:41

breaking out general interface

5d6e827

cleaning up exports

3b43484

colerogers changed the title ~~General Interface for FireAlerts~~ Generic Interface for enhanced provider Jan 19, 2022

colerogers requested review from inlined and taeold and removed request for inlined January 19, 2022 22:52

colerogers changed the title ~~Generic Interface for enhanced provider~~ Generic interface for provider enhancement Jan 20, 2022

colerogers added 3 commits January 20, 2022 07:27

removing comments & references to specific interfaces

eb8e4fb

format

b3f9365

jsdoc comments

d2c4a56

colerogers added the DO NOT MERGE label Jan 20, 2022

taeold requested changes Jan 20, 2022

View reviewed changes

taeold reviewed Jan 20, 2022

View reviewed changes

spec/v2/providers/alerts/alerts.spec.ts Outdated Show resolved Hide resolved

address pr comments

2102689

colerogers requested a review from taeold January 24, 2022 21:41

added param comment

ec42a87

taeold approved these changes Jan 24, 2022

View reviewed changes

spec/v2/providers/alerts/alerts.spec.ts Outdated Show resolved Hide resolved

src/v2/providers/alerts/alerts.ts Outdated Show resolved Hide resolved

src/v2/providers/alerts/alerts.ts Outdated Show resolved Hide resolved

addressing comments

8f92e1f

inlined approved these changes Jan 27, 2022

View reviewed changes

colerogers and others added 4 commits January 27, 2022 09:29

Specific interface for provider enhancement (1/3) (#1021)

01de80b

* adding in app distro changes * removing comments & addings package refs * jsdoc comments * fix comments * adding periods to doc strings * fix wording

adding import/export statement

c633bd9

Specific interface for provider enhancement (2/3) (#1022)

b51ec90

* adding in billing changes * removing comments & adding package refs * jsdoc comments * addressing pr comments * change handler doc string * changing to BillingEventHandler type * remove BillingEventHandler type

Specific interface for provider enhancement (3/3) (#1023)

4598383

* adding in crashlytics changes * comments & adding package refs * address comments and make doc strings better * add opts.retry to event trigger

colerogers merged commit 4aa39c4 into colerogers.providers.firealerts Jan 28, 2022

colerogers deleted the colerogers.alerts branch January 28, 2022 16:25

colerogers mentioned this pull request Jan 28, 2022

Add alerting provider to the Firebase Functions SDK #1024

Merged

colerogers added perf-h1-2022 and removed DO NOT MERGE labels Feb 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic interface for provider enhancement #1020

Generic interface for provider enhancement #1020

colerogers commented Jan 19, 2022 •

edited

Loading

taeold left a comment

taeold Jan 20, 2022

colerogers Jan 24, 2022

taeold Jan 20, 2022

taeold Jan 20, 2022

taeold Jan 20, 2022

taeold Jan 20, 2022 •

edited

Loading

colerogers Jan 24, 2022

taeold Jan 20, 2022

colerogers Jan 24, 2022

taeold Jan 20, 2022

colerogers Jan 24, 2022

taeold left a comment

inlined Jan 27, 2022

colerogers Jan 27, 2022

inlined Jan 27, 2022

Generic interface for provider enhancement #1020

Generic interface for provider enhancement #1020

Conversation

colerogers commented Jan 19, 2022 • edited Loading

taeold 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

taeold Jan 20, 2022 • edited Loading

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

taeold 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

colerogers commented Jan 19, 2022 •

edited

Loading

taeold Jan 20, 2022 •

edited

Loading