-
Notifications
You must be signed in to change notification settings - Fork 955
(feat) initializeServerApp support for App Hosting auto init #9151
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
🦋 Changeset detectedLatest commit: 8d93ef2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1Affected Products
Test Logs |
packages/app/src/api.ts
Outdated
/** | ||
* Creates and initializes a {@link @firebase/app#FirebaseServerApp} instance. | ||
* | ||
* @param config - Optional `FirebaseServerApp` configuration. | ||
* | ||
* @returns The initialized `FirebaseServerApp`. | ||
* | ||
* @public | ||
*/ |
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.
I think it's useful to add @throws
tag on functions that throw, but the other functions in app don't do this, so maybe it'd just make it inconsistent which may be worse- feel free to ignore.
@throws {AppError.INVALID_SERVER_APP_ENVIRONMENT} - ...
packages/app/src/api.ts
Outdated
|
||
if (_options) { | ||
if (_isFirebaseApp(_options)) { | ||
app = _options; |
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.
It looks like this is the only place the app
variable is used, probably don't need to initialize it with the let
above in line 256.
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.
Fixed.
): FirebaseServerApp { | ||
if (isBrowser() && !isWebWorker()) { | ||
// FirebaseServerApp isn't designed to be run in browsers. | ||
throw ERROR_FACTORY.create(AppError.INVALID_SERVER_APP_ENVIRONMENT); | ||
} | ||
|
||
if (_serverAppConfig.automaticDataCollectionEnabled === undefined) { |
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.
Are we getting rid of this? Should this go in the second branch of the if/else? Maybe also the third?
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.
Yes, thank you, nice catch!
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.
Fixed.
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!
Would this have any impact on https://firebase.devsite.corp.google.com/docs/app-hosting/firebase-sdks or other App Hosting pages? Or possibly https://firebase.google.com/docs/web/ssr-apps?
Yes, I think that page should be updated. Both the "Automatically initialize Firebase Admin SDK and web SDKs" and "Use FirebaseServerApp for SSR" sections. Hit up @jamesdaniels and me to get the ball rolling there.
I don't think this page needs any updates. It doesn't mention auto-initialization at all, and I don't think we want to push for its usage outside of App Hosting context. |
Discussion
Implement Auto Init for
initializeServerApp
.Auto init was previously implemented for
initializeApp
in #8483. This PR adds the same functionality to theinitializeServerApp
API surface.Fixes #8863.
Testing
Local testing in Node and a Next.js app.
API Changes
Internal doc: Firebase API - JS SDK initializeServerApp auto init