firebase / firebase-functions-test

MIT License
232 stars 48 forks source link

Type of makeDocumentSnapshot does not accept null value. #94

Closed kub1x closed 3 years ago

kub1x commented 3 years ago

Version info

firebase-functions-test:0.2.3

firebase-functions:3.13.2

firebase-admin:9.5.0

Test case

import * as myFunctions from '../src';
const wrapped = test.wrap(myFunctions.someFunction);

const data_path = 'myCollectionName/myDocId';
const beforeSnap = test.firestore.makeDocumentSnapshot(null, data_path); // <-- Rises a Typescript error
const afterSnap = test.firestore.makeDocumentSnapshot({ myprop: 'After Value' }, data_path);
const change = test.makeChange(beforeSnap, afterSnap);

await wrapped(change);

Steps to reproduce

The makeDocumentSnapshot with null as first parameter.

Expected behavior

Expect the makeDocumentSnapshot typescript type to allow me to pass null as first argument.

I'd expect the function to create document snapshot with exists set to false. Otherwise testing document creation/deletion with onWrite event handler won't be possible (like in this example).

Actual behavior

Calling the makeDocumentSnapshot with null rises typescript error:

Argument of type 'null' is not assignable to parameter of type '{ [key: string]: any; }'.
const beforeSnap = test.firestore.makeDocumentSnapshot(null, data_path);
kub1x commented 3 years ago

Found in other repo that manually setting the first argument to null as any allows me to pass null there and the test works that way.

As null seems to be a valid value of the argument, it probably should be allowed by the type definition. Current behavior feels misleading.

wceolin commented 3 years ago

Not sure if not accepting null is intentional or a bug but I noticed they recommend passing {} to mock the snapshot a document that doesn't exist.

taeold commented 3 years ago

Thanks @wceolin for sharing your comment.

@kub1x I think passing in an empty object is equivalent to what you want to achieve with passing in null. Is there something preventing you from doing that instead?

FWIW I don't know why makeDocumentSnapshot is designed this way, but I don't think it's a bug.

kub1x commented 3 years ago

Thanks, I've tested it and yes, it marks the empty object as exists === false too. A little counter intuitive, but works. I don't think I would have learned about this if I didn't ask here. Thanks!