Update Rtdb test sdk to allow json for CloudEvent #159

TheIronDev · 2022-08-03T19:48:17Z

Description

This commit updates the test sdk api for the Database CloudEventPartial
to support end-users submitting JSON that gets transformed into DataSnapshot
instead of forcing users to import and use Change and DataSnapshot to
mock out the CloudEvent resopnses.

Code sample

// Previous example
const cloudFn = database.onValueDeleted(referenceOptions, handler);
const cloudFnWrap = wrapV2(cloudFn);
const dataVal = { snapshot: 'override' };

// Calling makeDataSnapshot is the step we are trying to simplify
const data = makeDataSnapshot(dataVal, referenceOptions.ref);

cloudFnWrap({ data: data });

// New example
const cloudFn = database.onValueDeleted(referenceOptions, handler);
const cloudFnWrap = wrapV2(cloudFn);
const dataVal = { snapshot: 'override' };

cloudFnWrap({ data: dataVal });

This commit updates the test sdk api for the Database CloudEventPartial to support end-users submitting JSON that gets transformed into `DataSnapshot` instead of forcing users to import and use `Change` and `DataSnapshot` to mock out the CloudEvent resopnses.

src/cloudevent/generate.ts

src/cloudevent/mocks/database/helpers.ts

taeold · 2022-08-04T18:23:37Z

src/v2.ts

 */
 export type WrappedV2Function<T extends CloudEvent<unknown>> = (
-  cloudEventPartial?: DeepPartial<T>
+  cloudEventPartial?: DeepPartial<T | object>


I think typing of this should be DeepPartial<T> | Object instead.

Since Object is roughly similar to any, I'm pretty sad to see this function loosing quite a bit of type info. Is there way to make the typing a little more stricter? E.g. Object can only have keys that are part of CloudEvent type (I'm not sure if my suggestion makes complete sense).

But Object throws a lint error.

Thats a rough one. If we want to support raw JSON being passed in for the onValueCreated and onValueDeleted, I'm not sure of any good alternatives.. :\

TIL.

Understood. I wish there were a way to make this type-safe somehow (e.g. after/before example we discussed offline) but I don't see a way.

There's a big difference between Object (JS) and object (TS). For one, I don't know if DeepPartial<T> | object would be the same (IDK if you can even use "object" outside of "extends object"). If you really want to say "Anything that looks like a map" you can use Record<string, unknown>. If you want to come up with some really clever metaprogramming, I can try too.

Co-authored-by: Daniel Lee <danielylee@google.com>

taeold

lgtm but just one question so I understand this PR better:

In the new example, doesn't have to call makeDataSnapshot(data, ref). Does user NEED to specify ref property on their object? Or is it automatically inferred from... something else (maybe from cloud function definition? But that would almost always be wildcard ref right?)

TheIronDev · 2022-08-08T18:02:28Z

lgtm but just one question so I understand this PR better:

In the new example, doesn't have to call makeDataSnapshot(data, ref). Does user NEED to specify ref property on their object? Or is it automatically inferred from... something else (maybe from cloud function definition? But that would almost always be wildcard ref right?)

Ref gets automatically inferred and extracted here:

https://github.com/firebase/firebase-functions-test/blob/master/src/cloudevent/mocks/database/helpers.ts

inlined · 2022-08-10T02:32:33Z

src/cloudevent/generate.ts

+    : generatedCloudEvent;
+}
+
+function shouldDeleteUserSuppliedData<EventType extends CloudEvent<unknown>>(


This is such a cool problem you had to solve. Good find!

inlined · 2022-08-10T02:33:28Z

src/cloudevent/mocks/database/helpers.ts

 import { Change } from 'firebase-functions';
+import { makeDataSnapshot } from '../../../providers/database';
+
+type ChangeLike = {


nit: This reads a little weird. Why did you create a type alias to an anonymous interface type rather than just creating a named interface (i.e. interface ChangeLike {)

inlined · 2022-08-10T02:37:16Z

src/cloudevent/mocks/database/helpers.ts

+  if (data instanceof Change) {
+    return data;
+  }
+  if (data instanceof Object && data?.before && data?.after) {


I wonder if we want && (data?.before || data?.after) and make an undefined before/after be a DataSnapshot to a nonexistent value. WDYT?

Functionally it's the difference between one LOC in:

wrappedFn({ ref: "users/inlined" data: { // is this necessary? before: null, after: { name: "Thomas" }, } });

I could see arguments in either direction. Either we're helping them write simpler code or we could require them to be explicit to avoid accidental misuse (e.g. typo in "before" or "after"). Thoughts?

inlined · 2022-08-10T02:38:38Z

src/cloudevent/mocks/database/helpers.ts

  cloudFunction: CloudFunction<database.DatabaseEvent<database.DataSnapshot>>,
-  cloudEventPartial?: DeepPartial<database.DatabaseEvent<database.DataSnapshot>>
+  cloudEventPartial?: DeepPartial<
+    database.DatabaseEvent<database.DataSnapshot | object>


Nit: I personally would hoist | object to the top of the type definition because a DeepPartial<object> is meaningless. So DeepPartial<database.DatabaseEvent<database.DataSnapshot>> | object

inlined · 2022-08-10T02:45:52Z

src/v2.ts

 */
 export type WrappedV2Function<T extends CloudEvent<unknown>> = (
-  cloudEventPartial?: DeepPartial<T>
+  cloudEventPartial?: DeepPartial<T | object>


There's a big difference between Object (JS) and object (TS). For one, I don't know if DeepPartial<T> | object would be the same (IDK if you can even use "object" outside of "extends object"). If you really want to say "Anything that looks like a map" you can use Record<string, unknown>. If you want to come up with some really clever metaprogramming, I can try too.

TheIronDev requested review from colerogers, inlined and taeold August 3, 2022 19:48

taeold reviewed Aug 4, 2022

View reviewed changes

TheIronDev and others added 3 commits August 4, 2022 16:09

Updates per PR feedback

1481939

Update src/cloudevent/mocks/database/helpers.ts

eb3d89b

Co-authored-by: Daniel Lee <danielylee@google.com>

Added ChangeLike type, and reverted an Object to object (lint errror)

77b831b

TheIronDev requested a review from taeold August 5, 2022 15:40

taeold reviewed Aug 5, 2022

View reviewed changes

TheIronDev requested a review from taeold August 8, 2022 18:30

taeold approved these changes Aug 8, 2022

View reviewed changes

TheIronDev merged commit 6ee1a34 into master Aug 8, 2022

TheIronDev mentioned this pull request Aug 8, 2022

Update changelog for v2.3.0 #160

Merged

inlined reviewed Aug 10, 2022

View reviewed changes

Update Rtdb test sdk to allow json for CloudEvent #159

Update Rtdb test sdk to allow json for CloudEvent #159

Uh oh!

Conversation

TheIronDev commented Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Code sample

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taeold left a comment

Choose a reason for hiding this comment

Uh oh!

TheIronDev commented Aug 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TheIronDev commented Aug 3, 2022 •

edited

Loading