idField on valueChanges doesn't work as expected.

[ammaarpatel99]

Version info

Angular: 11

Firebase: 8

AngularFire: 6

Other: MacOS Big Sur

Issue

When you call valueChanges on a document you know doesn't exist, it emits undefined. However if you specify idField, for example, as id, then it will return an object with the field id. I expected that it would still return undefined. It returning causes problems, as the object then isn't in the shape you expect (if you specify a type of the result) but also you get a result when a document doesn't actually exist.

I think the issue is simply in the valueChanges function:

valueChanges<K extends string>(options: { idField?: K } = {}): Observable<T | undefined> {
  return this.snapshotChanges().pipe(
    map(({ payload }) =>
      options.idField ? {
        ...payload.data(),
        ...{ [options.idField]: payload.id }
      } as T & { [T in K]: string } : payload.data()
    )
  );
}

As you can see, if payload.data() is undefined, this function will create an object with id field. I think a simple fix would be:

valueChanges<K extends string>(options: { idField?: K } = {}): Observable<T | undefined> {
  return this.snapshotChanges().pipe(
    map(({ payload }) =>
      options.idField && payload.data() ? {
        ...payload.data(),
        ...{ [options.idField]: payload.id }
      } as T & { [T in K]: string } : payload.data()
    )
  );
}

I don't actually know much about the underlying firebase apis, but I'm assuming that payload.data() is undefined if there is no document.

[google-oss-bot]

This issue does not seem to follow the issue template. Make sure you provide all the required information.

[rgant]

But the document reference has an ID (that you queried to get the reference)... so why wouldn't it return the ID you requested?

[ammaarpatel99]

Because if the document doesn't actually exist, you would expect it to return nothing. Think of it this way, if you don't provide idField and the document doesn't exist, the observable returned emits undefined, not {}. So if the document doesn't exist, you expect undefined.
When you specify idField, you're basically saying that when you return the data, I want the id of the document to be added to it. However if there is no data, my opinion is it shouldn't be added. I found this because I had a guard that checked if a document exists, and if it does pass it onto the component. It made sense to have the id passed as well, as otherwise the component would have to check the route data again. However the document query returned an object for an invalid route (the document didn't exist but it still returned the object, with only the id field), so my guard thought the data existed and the component was made without the required data.

Long story short, this is about the intended behaviour of idField. If it is to add the id of a document to the returned data, then it makes sense that if there is no data, the id field shouldn't be returned either.

Do you understand what I'm saying, or is my explanation just going round in circles. I could try and make some examples to show what I mean if you want?

[ammaarpatel99]

I feel like returning the an object (with the id field) implies that the document exists.

If others don't feel this is the case, maybe an extra option could be added to specify this. This option would also prevent unexpected changes if people are used to the current behaviour.

[jamesdaniels]

Addressed in v7 api.