FR: improve typing on CRUD methods (again)

[erikbrinkman]

Describe your environment

• Operating System version: Windows 10 / Ubuntu 20.04 on WSL
• Browser version: Windows Chrome 87
• Firebase SDK version: 8.2.1
• Firebase Product: database

Describe the problem

Like in #1496, I think the typing around CRUD methods could be better, but I have some different / extended suggestions from that discussion that I think would be helpful, and that thread is currently locked.

First, in response to the general comments in that thread, typescript 4.1 is much more powerful. Here's a small example that does something close to what you may want for an update call. It allows any combination of paths or nested objects from the original, but nested objects must be complete. I don't necessarily claim that this is the best solution, but it does seem to work.

The second aspect that'd be good to handle in cases like these is support for FieldValues. What'd be ideal is a similar type transform that took things like number and transformed them into number | FieldValue.increment or Timestamp to Timestamp | FieldValue.serverTimestamp. I'm imagining something like this.

Finally the current way to apply types and type checking is to use a FirestoreDataConverter but that requires the same type for input and output. In the way this is presented in the docs this makes sense because often those are ES6 types where it might not be feasible to substitute a FieldValue or anything else. However, it's also feasible to use something sort of like a typescript interface, which seems like it should then allow FieldValues even when calling set vs update. I could buy that the argument is that the converter should handle the conversion from "write server timestamp" in the type to a fieldValue, and the type itself shouldn't handle FieldValues, but that seems a little restrictive. I'm not sure what makes the most sense to remedy this. One way would be to update the interface for set to take an appropriate union of types and FieldValues, but if the converter requires ES6 types then that would be too permissive.

In terms of what to actually change, the first two seem reasonable to apply to update in some capacity, but given the conversation in the past feature request, this still might not be enough to warrant updating the interfaces. For the last point, I'm curious to get feedback and maybe some more context around converters and types as it applies to database reads and writes.

Steps to reproduce:

Try calling CRUD interface calls in typescript.

Relevant Code:

See linked typescript playground links.

[schmidt-sebastian]

@erikbrinkman We have had many discussions about the update API both internally and externally, and had to reject most API improvements as they invalidated one of the use cases that we designed this API for. The code snippet you provided in the first example seems to now address our concerns and the desires that our users have been expressing. While it will take us some time to fully test the type interface you provided, it looks like it might work. Unfortunately, we will also have to carefully weigh whether this is a backwards compatible change. Right now, I tend to think that it is not and we may have to wait until we bump our major version next.

Your second idea (accepting FieldValues in FirestoreDataConverter and all input types, even if the user-provided type does not explicitly allow it) can probably be implemented without a breaking API change. We will explore this internally.

[schmidt-sebastian]

@erikbrinkman I discussed this with my team last week. We are generally in favor and would like to get these changes into our next breaking change release. For now, we have slightly different priorities though and we will keep you updated should this change slip. To answer the obvious follow-up question - we also don't have a confirmed date for the next major release yet. Fingers crossed and thanks for your help!

[erikbrinkman]

Thanks for your quick feedback! That sounds great. I appreciate you taking the time to review it internally, and I look forward to better static type checking in a future major release. If there's anything I can do to help, I'm happy to offer, but also just as happy to let you figure out what makes sense internally and on your own schedule.

Thanks again!

[goleary]

I've lately been lamenting the fact that we cannot use FirestoreDataConverters alongside firebase.firestore.FieldValues, so I'm excited to read this @schmidt-sebastian:

Your second idea (accepting FieldValues in FirestoreDataConverter and all input types, even if the user-provided type does not explicitly allow it) can probably be implemented without a breaking API change. We will explore this internally.

Also @erikbrinkman your TS 4.1 example looks great! I've also been thinking that template literal types could help out here.

Excited for whenever these things make it into the library!

[matt-kinton]

To add to this, although it might warrant it's own issue, could we get set to use a recursive NestedPartial type. Currently if I want to update a field in a nested object with { merge: true } Typescript complains because the type set requires is Partial which only ensures that the top level fields can be omitted.

[schmidt-sebastian]

That's a good suggestion. Thanks @Mattinton