Add ability to pass in env var into initializeApp#9904
Add ability to pass in env var into initializeApp#9904
Conversation
🦋 Changeset detectedLatest commit: 2270cec 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for initializing Firebase apps using JSON configuration strings in initializeApp and initializeServerApp. The changes include API updates, parsing logic with error handling, and unit tests. Review feedback suggests validating that the parsed JSON is an object to prevent runtime issues with primitives and correcting a documentation error in a function overload.
Changeset File Check ✅
|
| */ | ||
| export function initializeApp( | ||
| options: FirebaseOptions, | ||
| options: FirebaseOptions | string, |
There was a problem hiding this comment.
Why are the overloads that have "name" as a second argument separated into one whose first arg is options and one whose first arg is a string, but this one is combined?
| 'Unable to parse FirebaseOptions JSON string.' | ||
| ); | ||
| } | ||
| if (typeof parsed !== 'object') { |
There was a problem hiding this comment.
Is this supposed to check if it's a JS object? This will pass if it's an array. I'm not sure if this check is necessary as many other things could go wrong, and we should try to keep initializeApp as minimal as possible, as every user uses it.
| ); | ||
| } | ||
| if (typeof parsed !== 'object') { | ||
| throw new FirebaseError( |
There was a problem hiding this comment.
The firebase/app package doesn't instantiate FirebaseErrors directly but uses ERROR_FACTORY which uses a predefined error message template which has slots for certain strings to be inserted. The message probably wouldn't work for this case and is actually somewhat outdated. If you want to turn this PR over to me, I can rework it.
| "firebase": minor | ||
| --- | ||
|
|
||
| Add ability to pass in a Node environment variable into `initializeApp` and `initializeServerApp` |
There was a problem hiding this comment.
"Add option to pass Firebase config as a JSON string into initializeApp and initializeServerApp."
| if (typeof optionsOrJsonConfigString === 'string') { | ||
| let parsed: unknown = undefined; | ||
| try { | ||
| parsed = JSON.parse(optionsOrJsonConfigString); |
There was a problem hiding this comment.
Do we need this intermediate variable? Can't we just options = JSON.parse(optionsOrJsonConfigString)? Also does it need to be || undefined below? Does it mess something up if it's falsy but not specifically undefined?
| [AppError.INVALID_SERVER_APP_ENVIRONMENT]: | ||
| 'FirebaseServerApp is not for use in browser environments.' | ||
| 'FirebaseServerApp is not for use in browser environments.', | ||
| [AppError.INVALID_JSON_CONFIG]: 'Unable to parse FirebaseOptions JSON string.' |
There was a problem hiding this comment.
Let's show them the bad string. Use a template variable (it's a very weird bespoke format but you can see it in {$appName} and {$originalErrorMessage} above) and provide the bad string as the second arg when you call ERROR_FACTORY.create on this error type.
| expect(app.options).to.deep.equal(config); | ||
| }); | ||
|
|
||
| it('creates DEFAULT App with JSON config string and config object', () => { |
There was a problem hiding this comment.
"and config object" - no config object in this test, right?
| expect(app.name).to.equal(DEFAULT_ENTRY_NAME); | ||
| expect(app.options).to.deep.equal(config); | ||
| }); | ||
| it('creates DEFAULT app with config object as the second parameter', () => { |
There was a problem hiding this comment.
This one has a name, not DEFAULT.
| /Unable to parse FirebaseOptions JSON string/ | ||
| ); | ||
| }); | ||
| it('creates FirebaseServerApp with options as first parameter and config object with automaticDataCollectionEnabled as second parameter', async () => { |
There was a problem hiding this comment.
Is this identical to the test in 290? This one has a more specific description though.
3 participants
initializeAppas so: