Issue 2393 - Add environment check to Performance Module #3424
Conversation
…edDB not supported in some browser situations
…e build (#3156) * Do not build firestore lite in build because it breaks release build * add release build script
🦋 Changeset is good to goLatest commit: f558aa0 We got this. This PR includes changesets to release 9 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 |
Binary Size ReportAffected SDKs
Test Logs |
packages/performance/index.ts
Outdated
| }; | ||
| } | ||
| interface FirebaseApp { | ||
| performance?(): FirebasePerformance; | ||
| } | ||
| } | ||
|
|
||
| async function isSupported(): Promise<boolean> { |
There was a problem hiding this comment.
Performance is a monitoring sdk. It is easier for the users to initialize it in all environments, but performance will not start internally if all the apis it needs are not available.
Because of that, we probably do not need the isSupported function so the user can check before initializing.
There was a problem hiding this comment.
Analytics is in a similar situation but it throws if the environment conditions aren't met, on the assumption the developer might want to be alerted if they're not getting analytics data from a certain type of environment. isSupported() is a way to prevent initialization and avoid users having to see those errors. Maybe analytics could make some changes in the future?
But for now, since Performance is designed to surface this through console.info which won't appear for most users, I guess we don't need a mechanism to suppress it. Also performance internal initialization can be blocked by an async check (unlike analytics) so it seems like all the checks and blocking can be handled without developers or users seeing anything.
There was a problem hiding this comment.
I removed the isSupported function
…into issue-2393-performance
…firebase-js-sdk into issue-2393-performance
| @@ -109,11 +113,30 @@ export class Api { | |||
| ); | |||
| } | |||
|
|
|||
| requiredApisAvailable(): boolean { | |||
| if (fetch && Promise && this.navigator && this.navigator.cookieEnabled) { | |||
| async requiredApisAvailable(): Promise<boolean> { | |||
There was a problem hiding this comment.
The change looks good!
Can you please add a test for this function as it is not trivial anymore?
…into issue-2393-performance
| stub(FirebaseUtil, 'validateIndexedDBOpenable').throws(); | ||
| stub(api.navigator, 'cookieEnabled').value(true); | ||
| return api.requiredApisAvailable().then(() => { | ||
| expect(consoleLogger.info).to.to.be.called; |
There was a problem hiding this comment.
nit: one "to" can be removed.
| stub(consoleLogger, 'info'); | ||
| stub(api, 'navigator').value(null); | ||
| return api.requiredApisAvailable().then(() => { | ||
| expect(consoleLogger.info).to.be.called; |
There was a problem hiding this comment.
Can we check for the return value as well?
…into issue-2393-performance
…into issue-2393-performance
…flect changes of isRequiredApiAvailable
Issue 2393 fix for Performance module.