-
Notifications
You must be signed in to change notification settings - Fork 962
(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
Changes from 6 commits
4ee298c
ed091d2
4ff465a
07b8f26
238e4f9
832f81e
9f67976
b999c00
b43f513
1653cfc
7b1a506
8d93ef2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
--- | ||
'@firebase/app': minor | ||
'firebase': minor | ||
--- | ||
|
||
initializeServerApp now supports auto-initialization for Firebase App Hosting. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,7 @@ import { | |
_apps, | ||
_components, | ||
_isFirebaseApp, | ||
_isFirebaseServerAppSettings, | ||
_registerComponent, | ||
_serverApps | ||
} from './internal'; | ||
|
@@ -220,41 +221,62 @@ export function initializeApp( | |
* | ||
* @param options - `Firebase.AppOptions` to configure the app's services, or a | ||
* a `FirebaseApp` instance which contains the `AppOptions` within. | ||
* @param config - `FirebaseServerApp` configuration. | ||
* @param config - Optional `FirebaseServerApp` settings. | ||
* | ||
* @returns The initialized `FirebaseServerApp`. | ||
* | ||
* @public | ||
*/ | ||
export function initializeServerApp( | ||
options: FirebaseOptions | FirebaseApp, | ||
config: FirebaseServerAppSettings | ||
config?: FirebaseServerAppSettings | ||
): FirebaseServerApp; | ||
|
||
/** | ||
* Creates and initializes a {@link @firebase/app#FirebaseServerApp} instance. | ||
* | ||
* @param config - Optional `FirebaseServerApp` configuration. | ||
* | ||
* @returns The initialized `FirebaseServerApp`. | ||
* | ||
* @public | ||
*/ | ||
export function initializeServerApp( | ||
config?: FirebaseServerAppSettings | ||
): FirebaseServerApp; | ||
export function initializeServerApp( | ||
_options: FirebaseOptions | FirebaseApp, | ||
_serverAppConfig: FirebaseServerAppSettings | ||
_options?: FirebaseApp | FirebaseServerAppSettings | FirebaseOptions, | ||
_serverAppConfig: FirebaseServerAppSettings = {} | ||
): 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||
_serverAppConfig.automaticDataCollectionEnabled = true; | ||
let app: FirebaseApp; | ||
let firebaseOptions: FirebaseOptions | undefined; | ||
let serverAppSettings: FirebaseServerAppSettings = _serverAppConfig || {}; | ||
|
||
if (_options) { | ||
if (_isFirebaseApp(_options)) { | ||
app = _options; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like this is the only place the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||
firebaseOptions = app.options; | ||
} else if (_isFirebaseServerAppSettings(_options)) { | ||
serverAppSettings = _options; | ||
} else { | ||
firebaseOptions = _options; | ||
} | ||
} | ||
|
||
let appOptions: FirebaseOptions; | ||
if (_isFirebaseApp(_options)) { | ||
appOptions = _options.options; | ||
} else { | ||
appOptions = _options; | ||
firebaseOptions ||= getDefaultAppConfig(); | ||
dlarocque marked this conversation as resolved.
Show resolved
Hide resolved
hsubox76 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (!firebaseOptions) { | ||
throw ERROR_FACTORY.create(AppError.NO_OPTIONS); | ||
} | ||
|
||
// Build an app name based on a hash of the configuration options. | ||
const nameObj = { | ||
..._serverAppConfig, | ||
...appOptions | ||
...serverAppSettings, | ||
...firebaseOptions | ||
}; | ||
|
||
// However, Do not mangle the name based on releaseOnDeref, since it will vary between the | ||
|
@@ -270,7 +292,7 @@ export function initializeServerApp( | |
); | ||
}; | ||
|
||
if (_serverAppConfig.releaseOnDeref !== undefined) { | ||
if (serverAppSettings.releaseOnDeref !== undefined) { | ||
if (typeof FinalizationRegistry === 'undefined') { | ||
throw ERROR_FACTORY.create( | ||
AppError.FINALIZATION_REGISTRY_NOT_SUPPORTED, | ||
|
@@ -283,7 +305,7 @@ export function initializeServerApp( | |
const existingApp = _serverApps.get(nameString) as FirebaseServerApp; | ||
if (existingApp) { | ||
(existingApp as FirebaseServerAppImpl).incRefCount( | ||
_serverAppConfig.releaseOnDeref | ||
serverAppSettings.releaseOnDeref | ||
); | ||
return existingApp; | ||
} | ||
|
@@ -294,8 +316,8 @@ export function initializeServerApp( | |
} | ||
|
||
const newApp = new FirebaseServerAppImpl( | ||
appOptions, | ||
_serverAppConfig, | ||
firebaseOptions, | ||
serverAppSettings, | ||
nameString, | ||
container | ||
); | ||
|
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.