Skip to content

chore: Edited the query filter to remove .get() #377

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

Merged
merged 5 commits into from
Nov 28, 2023

Conversation

shweta345
Copy link
Contributor

Updated the code to resolve a customer feedback about understanding simple queries due to the .get() function.

@thatfiredev
Copy link
Member

Hey @shweta345
Can you please describe what issue the customer was running into? Links to public Github issues or internal bug tickets are welcome. Thanks!

@shweta345
Copy link
Contributor Author

@thatfiredev Internal bug ID that it targets is http://b/300413440. I was unsure if I should link it here :)

@thatfiredev
Copy link
Member

@shweta345 I do see the 2 simple queries that the customer seems to be talking about:

Screenshot 2023-11-28 at 11 22 29

But they don't seem to match with the query you're updating in this PR:

https://github.com/shweta345/snippets-node/blob/c234a3db471da94e5c9be8a29d856e4a8df9e31e/firestore/main/index.js#L590

@shweta345
Copy link
Contributor Author

shweta345 commented Nov 28, 2023

@thatfiredev This is the link that customer has shared: https://cloud.google.com/firestore/docs/query-data/queries#simple_queries. If you see the second snippet, it is the one that I edited. (Node.js language)

Copy link
Member

@thatfiredev thatfiredev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shweta345 Ah, my bad. I was looking at the wrong snippet (modular SDK)

One minor change needed before we merge this:

Co-authored-by: Rosário P. Fernandes <rosariofernandes51@gmail.com>
@shweta345
Copy link
Contributor Author

@thatfiredev Merged your edits. Thank you! :)

Copy link
Member

@thatfiredev thatfiredev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Thanks for updating this :)

@thatfiredev thatfiredev enabled auto-merge (squash) November 28, 2023 11:37
@thatfiredev thatfiredev merged commit e29c2c3 into firebase:master Nov 28, 2023
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

Successfully merging this pull request may close these issues.

2 participants