Skip to content
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

fix(firestore): do not return idField on valueChanges if no document #2777

Conversation

KingDarBoja
Copy link
Contributor

@KingDarBoja KingDarBoja commented Mar 14, 2021

Checklist

Description

This should provide a safe way of returning the idField on document method valueChanges as the current behaviour is explained at the linked issue. In this case, the idField should only ever be returned if the document exists.

Code sample

@KingDarBoja KingDarBoja changed the title fix(firestore): do not return idField on valueChanges if document doe… fix(firestore): do not return idField on valueChanges if no document Mar 14, 2021
@KingDarBoja
Copy link
Contributor Author

I have no idea why the test isn't working when it does locally on my machine 🤔

test_result

@KingDarBoja KingDarBoja force-pushed the fix/value-changes-with-optional-idField-if-document-not-exists branch from 967c17a to fe78f89 Compare March 14, 2021 17:17
@davideast
Copy link
Member

Hey @KingDarBoja! We really do appreciate the PR but this would constitute a breaking change if we merged it into the main codebase. We are working on a new version of the library (#2770) and we could add it in there in the future, but right now we need to keep the library stable until then.

Would you be interested in adding this change in the near future when we are ready to accept PRs against the new lib version? I'll ping you in this thread to let you know when we are ready.

@KingDarBoja
Copy link
Contributor Author

No worries! Feel free to ping me when you guys ready, I am also around at the Angular Discord server in any case I forgot to check this PR 😄

@jamesdaniels jamesdaniels added this to the 7.0 milestone Apr 6, 2021
@jamesdaniels
Copy link
Member

This should now be addressed in rxfire/firestore#doc and @angular/fire/firestore#docValue, we opted to avoid breaks in @angular/fire/compat/firestore as I consider that (largely) feature complete.

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

Successfully merging this pull request may close these issues.

4 participants