googleapis / googleapis/google-cloud-node
[FR] Improve public api for working with cloud functions triggered by Firestore
- Dominant language
- TypeScript
- Stars
- 3.2k
- Forks
- 712
- Avg merge
- 2d 3h
- Merged PRs (30d)
- 99
Description
# Improve public API for working with cloud functions triggered by Firestore
Lately I've been working on a set of cloud functions which are triggered by Firestore events (see [Docs](https://cloud.google.com/functions/docs/calling/cloud-firestore?hl=de)).
While it does work out in the first place, we do feel quite a struggle when migrating the first functions to Typescript, which somehow feels off.
If you think that this is a feasible issue and want me to help out please let me know and point me to the favored direction.
Specifically there are 2 main issues we are encountering with the firestore client sdk.
## Missing public types for triggered events
Given any triggered function (create, update, delete, write), the function should look like this.
```ts
exports.helloFirestore = (event, context) => {
console.log('Hello Firestore!');
};
```
Neither of those params (`event` or `context`) types is part of this package exported types.
It could be argued whether `@google-cloud/firestore` is the adequate module for such feature, but I can hardly imagine a better one.
As `@google-cloud/functions-framework` is scoped on `http` and `CloudEvent` handling I don't see it fit there either.
For the `event` parameter this is something that might be resolved quite easily, since all required types more or less do exist within the package and are already part of the [documentation](https://cloud.google.com/functions/docs/calling/cloud-firestore?hl=de).
I would propose to add an exported type to the main module.
```ts
interface FirestoreEvent {
oldValue: Document,
updateMask: DocumentMask,
value: Document
}
// Usage e.g.
import { FirestoreEvent } from '@google-cloud/firestore';
```
It could be discussed if the `context` parameter is also in the scope of this package. I am interested in your opinion here.
## Arguably unnecessary read operations
Mostly depending on your use case it feels very awkward to work with the data that is already present within the `event` parameter.
The official documentation also shows this:
```ts
exports.makeUpperCase = event => {
const resource = event.value.name;
const affectedDoc = firestore.doc(resource.split('/documents/')[1]);
const curValue = event.value.fields.original.stringValue;
const newValue = curValue.toUpperCase();
if (curValue !== newValue) {
console.log(`Replacing value: ${curValue} --> ${newValue}`);
return affectedDoc.set({
original: newValue,
});
} else {
// Value is already upper-case
// Don't perform a(nother) write to avoid infinite loops
console.log('Value is already upper-case.');
}
};
```
While the data is already present in the `event` parameter, accessing a simple value is very cumbersome.
Keep in mind that this object is very shallow. A real documents will be tremendously more complex.
Furthermore, only a `DocumentReference` is being created for the sole purpose of updating it.
If I intend to work with a simple object it would require an additional read operation, since there is no exposed api to the internal used protobuf converter.
In an ideal scenario I would expect this package to have a public accessible method to create a `DocumentSnapshot` by using the data of the `event` object.
```ts
class Firestore {
...
convert(document: Document): DocumentSnapshot {...}
// or directly from the event itself
convert(firestoreEvent: FirestoreEvent): DocumentSnapshot {...}
...
}
// Usage e.g
const docSnapshot = firestore.convert(event.value);
// or directly from the event itself
const docSnapshot = firestore.convert(event);
// Data Access should be simple now
const doc = docSnapshot.data();
```
Contributor guide
Assessment
This issue has not been assessed yet.