FR: improve typing on CRUD methods

[jorroll]

As a blanket concept, it would be very useful to update the SDK's document create and update methods to accept a type param to better type the passed in document argument.

For example, the first overload for the Firestore DocumentReference#update() method accepts a single firestore.UpdateData argument which is lightly typed.

Example usage:

documentRef.update(data.doc);

It would be awesome if a user could supply an optional type param to update which typescript would use to check the data.doc interface.

For example:

documentRef.update<{name: string; age: number}>(data.doc);

Typescript would ensure that data.doc had the interface {name: string; age: number}.

This general concept would be widely useful across the SDK and I do not think it would be a breaking change. My immediate desire is for Firestore related methods, but I believe the typing for the entire library could be improved in some places.

An implementation example for the first overload of the DocumentReference class:

update<T extends firestore.UpdateData = firestore.UpdateData>(value: T): Promise<void>;

Is this something you would accept PR's for?

[schmidt-sebastian]

We generally do not want to impose stricter type restrictions in our API layer than those that the SDK and the backend can validate. While your suggestion only narrows the set of input values, it doesn't actually change the input types that are accepted. It general, we have refrained from offering typed DocumentSnapshots and DocumentReferences since Firestore by design does not enforce schema.

For now, I would suggest that you rely on intermediary types to achieve a similar result:

const foo : Footype = {...};
documentReference.set(foo);

This should get you 90% of what you are asking and you even get to write a couple more lines of TypeScript code :)

I am going to close this issue for now, but feel free to chime in with more feedback.

[jorroll]

We generally do not want to impose stricter type restrictions in our API layer than those that the SDK and the backend can validate.

I'm suggesting an optional, developer chosen type restriction. Thus, the word "impose" seems inappropriate. I.e.

update<T extends firestore.UpdateData = firestore.UpdateData>(value: T): Promise<void>;

If someone doesn't supply a type argument, their consumption of the library would not change.

While your suggestion only narrows the set of input values, it doesn't actually change the input types that are accepted.

I didn't suggest that it would. You seem to be suggesting that using optional type arguments in a typescript project would be confusing for developers, but they are a fundamental part of the typescript language (and again, these are optional types--if someone doesn't know how to use them, they probably will not even know they exist).

It doesn't seem desirable to design a typescript library for developers who don't understand typescript (presumably, these developers would be consuming the library as javascript and this conversation is n/a to them). This being said, if the fear is that this change would be confusing for developers, that seems like a different conversation (admittedly, one that the Firebase team is in a much better position to decide).

For now, I would suggest that you rely on intermediary types to achieve a similar result

This is my current work around. I imagine this is what most developers do, which seems like a reason to implement a proper fix (aka generics).

you even get to write a couple more lines of TypeScript code :)

Lawl 😆

[mikelehen]

@thefliik Thanks for this suggestion and explaining all your reasoning! You make good points. You're right that this wouldn't impose additional restrictions for people that don't use it, but I do still have a couple concerns after talking this over with @schmidt-sebastian and other teammates:

First, I think this actually doesn't end up being a good candidate for the update() method because update() actually doesn't expect document-structured data. If you have a :

class Address { city: string, ... };
class User { address: Address, ... };

Then doc.update<User>(...) is actually misleading / wrong, since it'll reject doc.update<User>({'address.city', 'New York'}) which is a perfectly valid / expected thing to do.

In general the correct typing for update() really is { [fieldPath: string]: any }, rather than something more structured.

That said, your proposal would work better for set() which does accept document-structured data, and so set<User>({...}) could help you catch mistakes.

My remaining concern here is mostly that I'm not aware of other TypeScript libraries exposing generic type parameters just to help the API consumer "double-check" themselves (make sure their arguments match up with the type they meant them to be). And so I'm worried it could generate confusion if people expect the generic type parameter to actually do something, e.g. if they expect .set<User>(...) to filter out fields that aren't actually in User.

So ultimately we think this is a reasonable idea, but we're leaning against making it right now. It's the kind of thing we could change our minds on if we get more feedback or see this becoming a common pattern in other libraries, etc.

In any case, thanks again for the feedback! Keep it coming.

[jorroll]

And so I'm worried it could generate confusion if people expect the generic type parameter to actually do something

Well, I'm skeptical that optimizing a typescript library for "developers who don't understand typescript" will generate the most utility, but the Firebase team is obviously in a better position to make this call so I can accept this reasoning.

In general the correct typing for update() really is { [fieldPath: string]: any }, rather than something more structured.

While this is technically true, I'll argue that, in reality, this is a false assertion. This is because, in any given place in my application I generally know, specifically, what I'm updating right then, and the interface of that thing is almost always specific and definable (such as IUserUpdate) and never { [fieldPath: string]: any } (in my case I can concretely say that it is never { [fieldPath: string]: any }). I can understand and agree that { [fieldPath: string]: any } absolutely makes sense as the default typing for the library's update method, but it will almost always be appropriate for a library consumer to provide a more specific type.

I'll also add that, while I'm aware of the "address.street" method of updating a single nested field, I have never ended up making use of the syntax (though, admittedly, I only have one Firebase app in production). I'm sure I will make use of the syntax at some point, but I think that doc.update<User>(...) would be broadly useful for most use cases, even if it is not useful for every use case.

One reason why I haven't found "address.street" particularly useful, is because in the scenario when the user is updating their address, it's easier to simply present the user with a complete address form prefilled with their existing address, let them change something, and then persist the entire address object over again. I would guess that this is the way most developers would handle this scenario.

Anyway, I'll submit the first vote in favor of changing the the current typing strategy in the library. And again, if you ever do decide to move towards a greater use of generics, I imagine the related PR's would be ripe for community contribution.

[mikelehen]

Points well-taken. :-)

Regarding update() I'll just throw out that we get a lot of confusion from devs who accidentally do doc.update({address: { city: 'New York' } }) when they wanted doc.update({'address.city': 'New York'}). Since the former leads to accidental data loss, I'm sensitive to anything that would make this problem worse.

By the way, I just had a thought... Have you considered using a TypeScript cast? E.g.:

let user = { name: ..., address: ... };
doc.set(<User>user);

I think this might give you the same type-safety you want, but it would work in any context in TypeScript, without us modifying the Firebase SDK?

[jorroll]

By the way, I just had a thought... Have you considered using a TypeScript cast?

Unfortunately this does not give the same level of type safety. Take the following example (with strict: true):

interface IContactListUpdateDoc {
  primary?: boolean;
  name?: string;
  updatedAt: FirebaseFirestore.FieldValue;
}

// As desired, typescript *does not* error on this:

const updateDoc: IContactListUpdateDoc = {
  primary: false,
  updatedAt: timestamp(),
};

// As desired, typescript *does* error on this

const updateDoc: IContactListUpdateDoc = {
  primary: false,
  updatedAt: timestamp(),
  BAD_PROPERTY: 'mwahahahahaha',
}

// UNDESIRED, typescript *does not* error on this

const updateDoc = {
  primary: false,
  updatedAt: timestamp(),
  BAD_PROPERTY: 'mwahahahahaha',
} as IContactListUpdateDoc

Update

I'll note that the <IContactListUpdateDoc>{ updatedAt: timestamp() } and { updatedAt: timestamp() } as IContactListUpdateDoc formats are identical.

As @schmidt-sebastian pointed out earlier, the work-around for the time being is

const foo : Footype = {...};
documentReference.set(foo);

The only problem with this is that it seems needlessly verbose (though, as you pointed out, perhaps not needlessly if generic types end up causing too much trouble for the Firebase team). It's a shame though. While no-schema obviously has advantages, it also has disadvantages. Used properly, typescript interfaces can help to mitigate some of the disadvantages without a runtime performance impact.

[mikelehen]

I'm confused. I think your UNDESIRED case is also going to be a problem for your original proposal as well as for the suggested workaround. i.e. I'd expect TypeScript to accept both of these:

const updateDoc5: IContactListUpdateDoc = {
  primary: false,
  updatedAt: timestamp(),
  BAD_PROPERTY: 'mwahahahahaha',
};

doc.set<IContactListUpdateDoc>({
  primary: false,
  updatedAt: timestamp(),
  BAD_PROPERTY: 'mwahahahahaha',
});

[jorroll]

Well that's unexpected :(. You are correct that typescript accepts

doc.set<IContactListUpdateDoc>({
  primary: false,
  updatedAt: timestamp(),
  BAD_PROPERTY: 'mwahahahahaha',
});

Thankfully, Typescript does not accept this:

const updateDoc5: IContactListUpdateDoc = {
  primary: false,
  updatedAt: timestamp(),
  BAD_PROPERTY: 'mwahahahahaha',
};

So the work-around is appropriate (more than appropriate, it is the best solution for this problem at the moment). See simple stackblitz example.

This appears to be because typescript function input parameters are bivariant. The following issues are tracking the addition of stricter function parameter variance, which I think would address this issue: #10717, #18770.

There is actually still an advantage to supplying input paramater types vs simply type casting, but the advantage is greatly reduced from what I thought it was. For example:

interface IAddress {
  street: string;
  city: string
}

// expected: not allowed; actual: not allowed
doc.set<IAddress>({
  street: string,
});

// expected: not allowed; actual: allowed
doc.set({
  street: string,
} as IAddress);

When/if support for covariance eventually lands in typescript, I think this issue would be worth revisiting. But ya, at the moment its usefulness is limited.

Regarding update() I'll just throw out that we get a lot of confusion from devs who accidentally do doc.update({address: { city: 'New York' } })

Regarding that point, if a user attempted the following, an error would be thrown:

interface IUser {
  name: string;
  address: {
    street: string;
    city: string;
  }
}

const updateDoc: Partial<IUser> = {
  address: { street: '123 Street' }
}

doc.update(updateDoc)

This is because Partial<IUser> does not recursively make properties optional, so either the whole address interface would need to be satisfied, or the address must be absent. Calling out this strategy in the docs may help avoid some of these errors. I know these types of errors are specifically why I bother typing my CRUD params.

[mikelehen]

Oops! When I did the updateDoc5 intermediary type example, TypeScript happily accepted it, but it turns out I had a stale version of TypeScript installed globally. 😞 With the latest version, it's definitely rejected, nice.

Anyway, thanks very much for exploring all the options here and tracking down the bivariant issues, etc.! And thanks for the pointer on Partial<>. I'd actually never seen that feature. Nifty!