Skip to content

Proposal: New observable properties #1819

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
davideast opened this issue Aug 17, 2018 · 12 comments
Closed

Proposal: New observable properties #1819

davideast opened this issue Aug 17, 2018 · 12 comments

Comments

@davideast
Copy link
Collaborator

davideast commented Aug 17, 2018

Properties not methods!

We’re thinking about introducing a new properties on the AngularFireCollection and AngularFireDocument classes.

const itemsCollection = afs.collection(‘items’, { includeMetadataChanges: false });
const itemSnap$ = itemsCollection.querySnapshot;
const items$ = itemsCollection.docs;
const itemsChanges$ = itemsCollection.docsChanges;

items$.subscribe(snapshot => {
  console.log(snapshot.docs);
  console.log(snapshot.docChanges);
});

items$.subscribe(docs => {
  const first = docs[0];
  console.log(first.data());
});

itemsChanges$.subscribe(changes => {
  const first = changes[0];
  console.log(first.type, first.data());
});

This helps avoid the sticky situation of loops. If you create an observable as a function inside a loop each render creates multiple observables with multiple subscriptions. If each observable has a different query criteria it will act unpredictably.

<!-- DONT DO THIS! -->
<ul>
  <li *ngFor=”let item of itemsCollection.snapshotChanges() | async”></li>
</ul>

Using a propery ensures that it’s only one instance of the observable, eliminating the unpredictable situations.

<!-- SO MUCH BETTER! -->
<ul>
  <li *ngFor=”let item of itemsCollection.docs | async”></li>
</ul>

We'll also add this for documents.

const itemDoc = afs.document(‘items/1, { includeMetadataChanges: false });
const item$ = itemDoc.doc;

item$.subscribe(doc => {
  console.log(doc.data());
});

Leave a comment and let us know what you think of this proposal.

@davideast davideast changed the title Proposal: New observable properties on AngularFireCollection Proposal: New observable properties Aug 17, 2018
@codediodeio
Copy link
Contributor

I like this and it aligns better with other modules in Angular like reactive forms valueChanges or the rotuer's queryParams.

One thing I'm not clear on... will the docs prop map the call to data() for you? Otherwise I believe you will get a "circular JSON" error with the async pipe, unless you map it yourself with RxJS, which is painful.

@davideast
Copy link
Collaborator Author

The docs prop will not map the call to data(). There's good information on the DocumentSnapshot so I don't want to strip it.

I'm thinking we can port your operators over from RxFire to do the unwrapping (#1820). What do you think?

@codediodeio
Copy link
Contributor

codediodeio commented Aug 17, 2018

Ah, that makes more sense. I think that's a reasonable way to go. The alternative would another prop like itemsCollection.data. A little less code, but not quite as flexible as Rx operators.

@davideast
Copy link
Collaborator Author

@codediodeio itemsCollection.data would only work if you configured it in AngularFireList:

const col = afs.collection(‘items’, { includeMetadataChanges: false }, { id: 'id' });
col.data.subscribe(items => {
  const first = items[0];
  console.log(first.id);
});

What do you think?

@pandamorphism
Copy link

any updates on this?

@jimmykane
Copy link

Guys why do you close the issues on a feature request that was never delivered?

@hiepxanh
Copy link
Contributor

hiepxanh commented Sep 3, 2019

any update on this sir?

@johnzondr
Copy link

Can we please get a fix for this?

@kylecordes
Copy link

A related suggestion for this feature: in the meantime, if there is any hesitation because this is an API change, edit the code to ensure that (for example) .snapshotChanges() returns the same observable each time and not a new one. (It might already do this correctly, in which case, perhaps this API change might simply be abandoned as unnecessary.)

@hiepxanh
Copy link
Contributor

hiepxanh commented Apr 4, 2020

@davideast any update on this sir?

@WebMex
Copy link

WebMex commented Nov 18, 2020

Is it already implemented? Is it possible to use includeMetadataChanges now?

@jamesdaniels
Copy link
Member

metadata changes are always included in snapshotChanges as of 6.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants