-
Notifications
You must be signed in to change notification settings - Fork 391
Scaffolding out extensions namespace #1829
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
Conversation
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.
LGTM! Thank you @joehan !
I added a few nit-picking comments below.
@@ -0,0 +1,39 @@ | |||
/*! | |||
* @license | |||
* Copyright 2021 Google Inc. |
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.
nit: should this be 2022?
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ |
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.
nit: new line
import { FirebaseApp } from '../app/firebase-app'; | ||
import { HttpClient, AuthorizedHttpClient } from '../utils/api-request'; | ||
import { FirebaseAppError } from '../utils/error'; | ||
import * as validator from '../utils/validator'; |
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.
nit: new line
@@ -0,0 +1,16 @@ | |||
/*! | |||
* @license | |||
* Copyright 2021 Google Inc. |
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.
nit: Should this be 2022?
const firebaseApp: FirebaseApp = app as FirebaseApp; | ||
return firebaseApp.getOrInitService('extensions', (app) => new Extensions(app)); | ||
} | ||
|
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.
nit: new line
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.
Thanks for catching these - gonna fix them in #1838 since they're all small style things
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.
LGTM
* Scaffolding out extensions namespace (#1829) * starting scaffolding * Finish scaffolding extensions * adding whitespace * Implements Extensions namespace (#1838) * starting scaffolding * Finish scaffolding extensions * adding whitespace * Implements Extensions namespace * Expose extensions module * fixing api-extractor by adding @internal * Improve error handling * lint * Add jsdocsand api-extract * merging * style fixes from 1829 * style fix * Addressing PR comments * Clean up getRuntimeData * typo fix * in the tests as well * PR fixes * round 2 of fixes * PR fixes * Update src/extensions/extensions.ts Co-authored-by: Kevin Cheung <[email protected]> * Update src/extensions/extensions.ts Co-authored-by: Kevin Cheung <[email protected]> * Update src/extensions/extensions.ts Co-authored-by: Kevin Cheung <[email protected]> * Update src/extensions/extensions.ts Co-authored-by: Kevin Cheung <[email protected]> * Docs pass * lint * Fix test Co-authored-by: Kevin Cheung <[email protected]> Co-authored-by: Kevin Cheung <[email protected]>
Scaffolding out the files for the extensions namespace described in go/firex-admin-sdk. Most of the real business logic for this is implemented in #1838 - I just split this out to keep things a bit more manageable for review.
DO NOT MERGE yet