AFS document update type error and custom object error

[codediodeio]

Issue

1. Firestore doc update method raises a type error when passing individual properties.
2. This method will also raise an error on custom JS objects.

Version info

AngularFire 5.0.0

Steps to Reproduce

/// 1. Type error issue
interface IUser {
  name: string;
  age: number;
}

const userDoc: AngularFirestoreDocument<IUser> = this.afs.doc('user/someId') ;

userDoc.update({ name: 'someName' }) /// not assignable to type IUser

/// 2. Custom object error issue

class User {
  constructor(public name: string, public age: number){}
}

userDoc.update(new User('hello', 23) )  /// invalid data error

Expected behavior

1. No type error when passing individual params to update
2. No errors when passing custom JS objects to update.

Proposed fix: remove generic and add spread to data argument in firestore/document.ts

  /**
   * Update some fields of a document without overwriting the entire document.
   * @param data
   */
  update(data): Promise<void> {
    return this.ref.update({ ...data });
  }

[Toxicable]

The data you're passing to the store must be an Object, not a class instance.
The reason I would assume for this is because while firebase can take the data out of a class it cannot re-instantiate that class once it comes back into the app from the store

Using plain Objects + interfaces is the method i'd advise

[codediodeio]

@Toxicable that makes sense, but it seems like AngularFire could convert instances implicitly without harm? For example, you have PhoneNumber class you instantiate, then send that to update the userDocument - you wouldn't expect an instance back, but also wouldn't expect an error.

What do you think about removing the generic type on the data argument, it was an Object type in previous versions?

[Toxicable]

That wouldn't be inconsistent if you don't get an instance back, what if the class has a method on it? you'd be able to use that method on the out going object but not in the incoming one, this would be very unintuitive.

Also the type Object does allow classes see example below, as far as I know there's not type in TS that disallows class instances

const f: (data: Object) => any;
class T { }
var t: T;

f(t); //no error

[codediodeio]

That's how update should work. If it's typed to a generic it's almost identical to set because you need to send all object data to the method based on its interface (even if updating one property). If that's the case, it should not be documented like this: "Update some fields of a document without overwriting the entire document"

[larssn]

Well still, it feels a bit silly.

It's nice being able to do:
const order = new Order(); and get default values for certain fields filled out automatically (for complex objects).

My current (silly) workaround is collection.add(JSON.parse(JSON.stringify(order)));

[codediodeio]

@larssn An easier solution would be the spread syntax, collection.add({ ...order }). However, I'm still in favor of AF2 doing this by default - that's how v4 worked.

[larssn]

@codediodeio I don't think that works for deep objects

[codediodeio]

@larssn Nope, one-level.

[lakinduakash]

Well still, it feels a bit silly.

It's nice being able to do:
const order = new Order(); and get default values for certain fields filled out automatically (for complex objects).

My current (silly) workaround is collection.add(JSON.parse(JSON.stringify(order)));

Still this workaround for nested objects?

[larssn]

@lakinduakash Yes