feat(firestore): Added option to include document IDs on valueChanges() #1976

codediodeio · 2018-12-19T03:43:23Z

Checklist

Issue number for this PR: Proposal: Optionally return a document ID on valueChanges() #1973
Docs included?: Yes
Test units included?: Yes
In a clean directory, yarn install, yarn test run successfully? Yes

Description

An 'idField' string arg can be used with collection.valueChanges() to include the document ID on the emitted data payload.

Code sample

collectionRef.valueChanges('myIdKey').subscribe()

// emits [ { myIdKey: 'MrfFpRBfWLTd7LqiTt9u', ...data }, ... ]

An 'idField' string arg can be used with collection.valueChanges() to include the document ID on the emitted data payload.

markgoho

LGTM

jamesdaniels

LGTM. Maybe we can union the return type T with [idField]: string if possible? But that can be follow on work.

davideast

Great work!

Just a little comment on the API.

davideast · 2019-01-11T19:12:41Z

src/firestore/collection/collection.ts

@@ -104,12 +104,18 @@ export class AngularFirestoreCollection<T=DocumentData> {
  /**
   * Listen to all documents in the collection and its possible query as an Observable.
   */
-  valueChanges(): Observable<T[]> {
+  valueChanges(idField?: string): Observable<T[]> {


Maybe something like this for future proofing? Not a huge deal but it could help.

Suggested change

valueChanges(idField?: string): Observable<T[]> {

valueChanges({ idField: string }: ValueChangeOptions?): Observable<T[]> {

I like that 👍

I second using an options object for future proofing. Great PR otherwise.

jorroll · 2019-01-25T16:30:14Z

@codediodeio I just created a new PR to update this PR and make valueChanges() accept an options object for idField instead of a single string argument -- to future proof things.

If you accept this change, it will (hopefully) make this PR here able to be merged.

feat(firestore): changed valueChanges() arg to options object

googlebot · 2019-05-10T02:37:56Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-05-10T02:37:56Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

jorroll · 2019-05-10T02:47:09Z

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

I am ok with my commits being contributed to this project 👍 !

@davideast per your comment, valueChanges() has been updated to accept an options object rather than a simple string valueChanges(options?: { idField: string }): Observable<T[]>.

cc @jamesdaniels

googlebot · 2019-05-11T00:31:10Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

googlebot · 2019-05-11T00:31:10Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

googlebot · 2019-05-11T01:12:03Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-05-11T01:12:03Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-05-11T01:12:41Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

googlebot · 2019-05-11T01:12:41Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

jamesdaniels · 2019-05-11T01:26:03Z

Thanks @codediodeio and @thefliik, will ship in the next beta and this will land in the 5.2 release.

markgoho · 2019-05-13T13:44:45Z

src/firestore/collection/collection.spec.ts

+      const ITEMS = 1;
+      const { ref, stocks, names } = await collectionHarness(afs, ITEMS);
+      const idField = 'myCustomID';
+      const sub = stocks.valueChanges({idField}).subscribe(data => {


I believe they are destructuring the valueChanges object and grabbing just the idField property.

{idField} is shorthand for {idField: idField}
https://stackoverflow.com/questions/34414766/ Shorthand property names (ES2015)

rtpHarry · 2020-01-05T19:02:35Z

thanks... you just ruined all the tutorials out there :)

Crypt0Graphic · 2020-01-17T21:31:58Z

When i use valueChanges({ idField: 'id' }) and map i get an error like below. If i directly subscribe to it there is no problem.

No problem with snapshotChanges() in both situation.

  getList() {
    return this.af.collection('Clubs', ref => ref.orderBy('name')).valueChanges({ idField: 'id' });
  }

    const newarray = [];
    this.subscription = this.clubsService
      .getList()
      .pipe(
        switchMap(val => from(val)),
        map(x => {
          const modified = { ...x, fCount: x.followers.length };
          newarray.push(modified);
          return newarray;
        })
      )

**error TS2339: Property 'followers' does not exist on type '{ id: string; }'.**

feat(firestore): Added option to include document IDs on valueChanges()

45dcb64

An 'idField' string arg can be used with collection.valueChanges() to include the document ID on the emitted data payload.

googlebot added the cla: yes label Dec 19, 2018

markgoho approved these changes Dec 22, 2018

View reviewed changes

jamesdaniels approved these changes Jan 3, 2019

View reviewed changes

davideast reviewed Jan 11, 2019

View reviewed changes

feat(firestore): changed valueChanges() arg to options object

0457903

chore(docs): fix Collection#valueChanges() docs

810cff5

jorroll mentioned this pull request Feb 8, 2019

Firestore: add option to get an observable of doc(s) with id #2005

Closed

jamesdaniels added this to the 5.2.0 milestone Feb 22, 2019

Merge pull request #1 from thefliik/collection-idFieldUpdate

4a4a1f5

feat(firestore): changed valueChanges() arg to options object

googlebot added cla: no and removed cla: yes labels May 10, 2019

jorroll approved these changes May 10, 2019

View reviewed changes

Merge branch 'master' into collection-idField

b31776e

jamesdaniels added cla: yes and removed cla: no labels May 11, 2019

Adding idField into the return type, if provided

d1dfa42

googlebot added cla: no and removed cla: yes labels May 11, 2019

jamesdaniels added cla: yes and removed cla: no labels May 11, 2019

jamesdaniels merged commit 7108875 into angular:master May 11, 2019

EdricChan03 reviewed May 13, 2019

View reviewed changes

peterennis mentioned this pull request May 14, 2019

feat(firestore): Added option to include document IDs on valueChanges… peterennis/angularfire2#28

Merged

jamesdaniels mentioned this pull request May 24, 2019

Discussion: 5.2 release #2086

Closed

flauc mentioned this pull request Jun 2, 2019

Simplify to valueChanges where possible Jaspero/fireshop#179

Closed

joeyaurel mentioned this pull request Jun 12, 2019

Proposal: Optionally return a document ID on valueChanges() when reading a single document #2109

Closed

mjastrzebowski mentioned this pull request Jun 15, 2019

feat(firestore): options to include document ID on valueChanges() #2113

Merged

sarunint mentioned this pull request Jul 4, 2019

feat(database): add option to include key on valueChanges() #2126

Merged

joeyaurel mentioned this pull request Aug 31, 2019

Noob Question. object.valueChanges vs object.snapshotChanges #2106

Closed

rtpHarry mentioned this pull request Jan 5, 2020

Proposal: Optionally return a document ID on valueChanges() #1973

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(firestore): Added option to include document IDs on valueChanges() #1976

feat(firestore): Added option to include document IDs on valueChanges() #1976

codediodeio commented Dec 19, 2018

markgoho left a comment

jamesdaniels left a comment

davideast left a comment

davideast Jan 11, 2019

codediodeio Jan 11, 2019

jorroll Jan 24, 2019

jorroll commented Jan 25, 2019 •

edited

Loading

googlebot commented May 10, 2019

googlebot commented May 10, 2019

jorroll commented May 10, 2019

googlebot commented May 11, 2019

googlebot commented May 11, 2019

googlebot commented May 11, 2019

googlebot commented May 11, 2019

googlebot commented May 11, 2019

googlebot commented May 11, 2019

jamesdaniels commented May 11, 2019

This comment was marked as resolved.

markgoho May 13, 2019

anisabboud Jun 18, 2019

rtpHarry commented Jan 5, 2020

Crypt0Graphic commented Jan 17, 2020 •

edited

Loading

	valueChanges(idField?: string): Observable<T[]> {
	valueChanges({ idField: string }: ValueChangeOptions?): Observable<T[]> {

feat(firestore): Added option to include document IDs on valueChanges() #1976

feat(firestore): Added option to include document IDs on valueChanges() #1976

Conversation

codediodeio commented Dec 19, 2018

Checklist

Description

Code sample

markgoho left a comment

Choose a reason for hiding this comment

jamesdaniels left a comment

Choose a reason for hiding this comment

davideast left a comment

Choose a reason for hiding this comment

davideast Jan 11, 2019

Choose a reason for hiding this comment

codediodeio Jan 11, 2019

Choose a reason for hiding this comment

jorroll Jan 24, 2019

Choose a reason for hiding this comment

jorroll commented Jan 25, 2019 • edited Loading

googlebot commented May 10, 2019

googlebot commented May 10, 2019

jorroll commented May 10, 2019

googlebot commented May 11, 2019

googlebot commented May 11, 2019

googlebot commented May 11, 2019

googlebot commented May 11, 2019

googlebot commented May 11, 2019

googlebot commented May 11, 2019

jamesdaniels commented May 11, 2019

This comment was marked as resolved.

markgoho May 13, 2019

Choose a reason for hiding this comment

anisabboud Jun 18, 2019

Choose a reason for hiding this comment

rtpHarry commented Jan 5, 2020

Crypt0Graphic commented Jan 17, 2020 • edited Loading

jorroll commented Jan 25, 2019 •

edited

Loading

Crypt0Graphic commented Jan 17, 2020 •

edited

Loading