Local dev firebase key #733
Conversation
amber-emmes
requested review from
JoeArmani and
bransteitterbr
and removed request for
a team
index.js
Outdated
| // Get the API key file from Box or the DevOps team | ||
| // Do not accept PRs with the localDevFirebaseConfig import uncommented | ||
| // import { firebaseConfig as localDevFirebaseConfig} from "./src/local-dev/config.js"; | ||
| const localDevFirebaseConfig = stageFirebaseConfig; |
There was a problem hiding this comment.
I'm not sure I understand...does this point dev at the stage config until the developer comments it out and activates the line above?
What about just omitting this line const localDevFirebaseConfig = stageFirebaseConfig; and throwing a descriptive error in the else if block to avoid the config crossover?
else if (isLocalDev) {
if (!localDevFirebaseConfig) {
throw new Error("localDevFirebaseConfig is missing. Update your config per team specs.");
}
!firebase.apps.length ? firebase.initializeApp(localDevFirebaseConfig()) : firebase.app();
}There was a problem hiding this comment.
It was my experience that trying to require a nonexistent file resulted in an error where the page refused to load entirely, so it wouldn't allow it to get that far in the code.
There was a problem hiding this comment.
I see. That leads to a bit of a cryptic error either way. Here's the error when we set it to stage and try to log in.
My concern is that this error would pile on top of other hard-to-diagnose errors we've seen when auth goes down or other parts of the system get interrupted. I know it's a nitpick for the everyday case, but I think it helps diagnose the problem when we have more challenging access issues. I know there have been times when the firebase emulator integration for full stack work has taken hours to debug.
A type check on localDevFirebaseConfig does allow us to show a specific error message while omitting line 9 (the reference pointing from dev -> stage). My opinion is feel like that would be simpler and helpful for debugging when more complex access issues arise.
} else if (isLocalDev) {
if (typeof localDevFirebaseConfig === 'undefined') {
console.error('Local development requires a localDevFirebaseConfig function to be defined in src/local-dev/config.js.');
return;
}
!firebase.apps.length ? firebase.initializeApp(localDevFirebaseConfig()) : firebase.app();
} else {
I'll approve and leave it up to you...that's my thought for all three integrations. Thanks
index.js
Outdated
| // Get the API key file from Box or the DevOps team | ||
| // Do not accept PRs with the localDevFirebaseConfig import uncommented | ||
| // import { firebaseConfig as localDevFirebaseConfig} from "./src/local-dev/config.js"; | ||
| const localDevFirebaseConfig = stageFirebaseConfig; |
There was a problem hiding this comment.
I see. That leads to a bit of a cryptic error either way. Here's the error when we set it to stage and try to log in.
My concern is that this error would pile on top of other hard-to-diagnose errors we've seen when auth goes down or other parts of the system get interrupted. I know it's a nitpick for the everyday case, but I think it helps diagnose the problem when we have more challenging access issues. I know there have been times when the firebase emulator integration for full stack work has taken hours to debug.
A type check on localDevFirebaseConfig does allow us to show a specific error message while omitting line 9 (the reference pointing from dev -> stage). My opinion is feel like that would be simpler and helpful for debugging when more complex access issues arise.
} else if (isLocalDev) {
if (typeof localDevFirebaseConfig === 'undefined') {
console.error('Local development requires a localDevFirebaseConfig function to be defined in src/local-dev/config.js.');
return;
}
!firebase.apps.length ? firebase.initializeApp(localDevFirebaseConfig()) : firebase.app();
} else {
I'll approve and leave it up to you...that's my thought for all three integrations. Thanks
|
I see a bit more steps needed for local tests, but I don't see how this prevents abuse. |
Part of 1054.