-
-
Notifications
You must be signed in to change notification settings - Fork 134
Update Firestore Indices File (#1717) #1721
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - I just had one minor (but long-winded) point about the wording on the deploy checklist update. Should be good to merge if we adjust that.
@@ -1,30 +1,5 @@ | |||
{ | |||
"indexes": [ | |||
//// Testimony Listing //// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note for posterity: We're fine removing the comments here - the context is mostly obvious from the collection/fields being indexed and they'll clash with the firebase firestore:indexes
output if we ever need to refresh these indexes from source again.
pull_request_template.md
Outdated
@@ -5,6 +5,7 @@ _Add a short summary of the changes, and a reference to the original issue using | |||
# Checklist | |||
|
|||
- [ ] On the frontend, I've made my strings translate-able. | |||
- [ ] On the backend, I've updated `firestore.indexes.json` with new indexes (`firebase firestore:indexes > firestore.indexes.json`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this as a start, but I think we should be clear that we need to actually create the new indexes first for this command to do anything.
Come to think of it, we should think through the desired deploy process here. There are a few problems:
- Indexes take a few minutes to create - and the
firebase deploy
process doesn't naturally block until indexes are created to finish deploying the queries that use them- This was a bit of a WTF moment for me (firebase cli: wait for Firestore indexes deployment firebase/firebase-tools#7959)
- It's easy to forget to add indexes when developing locally because the Firebase emulators and flawed and don't check indexes at all (so you can test fully while running locally and your new query will work fine, but it will blow up when it hits production).
- There is some tooling to try and sync local dev with the state of the real environment indexes, but it's unsupported and I'm not sure it's worth the effort to maintain just yet.
- The best way to test them AFAIK is to open a firebase console against the dev environment (with
yarn firebase-admin -e dev console
) and try to run your query there - it seems like the easiest way to tell if new queries need a new index.
For now, it might be easier just to explicit build out a manual deploy checklist and ensure we hit the steps:
TL;DR
This is a longwinded way to say: Let's break this into three smaller checkboxes that are something like:
- For any new firestore queries, are all new required indexes:
- Created on
development
- Created on
production
- Added to
firestore.indexes.json
(either manually or with the commandfirebase firestore:indexes > firestore.indexes.json
)
- Created on
One last note: most devs don't have access to the production
environment - so this would really be a step for PR reviewers if we move it out of the deploy automation.
After typing all this, it may be worth adding a note to the Wiki about how to add new firestore queries.
Summary
The version of
firestore.indexes.json
in themaple
repo root isn't updated automatically, and it becomes out of sync with the indexes in the firebase project when developers add new commands through the online console.This is a one-time update to bring the repo's version of
firestore.indexes.json
in line withfirebase firestore:indexes > firestore.indexes.json
, and an update to the PR template to remind backend developers to add their Firestore indexes if they've made updates.Checklist
Screenshots
N/A
Known issues
N/A
Steps to test/reproduce
N/A