Issue 2393 - Add environment check to Analytics Module #3165

XuechunHou · 2020-06-04T18:25:56Z

a fix for issue #2393 for analytics module.

Throw runtime error (UNSUPPORTED_BROWSER) on initialization of the instance when IndexedDB is not supported in some browser situations.

The fix is tested on Safari and Firefox private window rendering iframe with https://github.com/tomsun/firebase-js-sdk-securityerror-example provided in the issue conversation.

…e build (#3156) * Do not build firestore lite in build because it breaks release build * add release build script

google-oss-bot · 2020-06-04T18:33:59Z

Binary Size Report

Affected SDKs

@firebase/analytics

Type Base (c2b737b) Head (3035605) Diff

esm2017 8.68 kB 9.66 kB +975 B (+11.2%)

main 9.69 kB 10.9 kB +1.20 kB (+12.3%)

module 9.37 kB 10.6 kB +1.22 kB (+13.0%)

`@firebase/util`

Type	Base (`c2b737b`)	Head (`3035605`)	Diff
browser	19.6 kB	20.5 kB	+921 B (+4.7%)
esm2017	17.5 kB	18.3 kB	+786 B (+4.5%)
main	19.6 kB	20.5 kB	+921 B (+4.7%)
module	18.7 kB	19.5 kB	+832 B (+4.5%)

firebase

Type Base (c2b737b) Head (3035605) Diff

firebase-analytics.js 26.6 kB 28.0 kB +1.37 kB (+5.1%)

firebase.js 819 kB 820 kB +1.37 kB (+0.2%)

Test Logs

Base (c2b737b2): https://github.com/firebase/firebase-js-sdk/actions/runs/169202539
Head (30356059): https://github.com/firebase/firebase-js-sdk/actions/runs/169248357

…into issue-2393-analytics

…tory is passed

hsubox76

Looking good, left some comments.

Assuming this is WIP and you will remove extra code comments and console logs in a later pass.

hsubox76 · 2020-06-23T20:53:29Z

packages/analytics/index.ts

@@ -45,6 +45,9 @@ declare global {
 * Type constant for Firebase Analytics.
 */
 const ANALYTICS_TYPE = 'analytics';
+const NAMESPACE_EXPORTS = {


I assume this is copied from messaging, but I don't think it's necessary here. In messaging, this was the top-level object passed to setServiceProps(). Here, the top level object is a literal that currently contains settings and EventName, so you're adding another key named NAMESPACE_EXPORTS which contains this nested object that contains isSupported(). Have you manually tested calling the isSupported() static method? Did this work?

hsubox76 · 2020-06-23T20:54:41Z

packages/analytics/src/errors.ts

+  [AnalyticsError.INDEXED_DB_UNSUPPORTED]:
+    'IndexedDB is not supported by current browswer',
+  [AnalyticsError.INVALID_INDEXED_DB_CONTEXT]:
+    "Environment doesn't support IndexedDB functionality with error message: {$errorInfo}"


Don't think we need the words with error message, just ...functionality: {$errorInfo}

hsubox76 · 2020-06-23T20:56:35Z

packages/analytics/index.ts

  }
  interface FirebaseApp {
    analytics(): FirebaseAnalytics;
  }
 }
+function validateBrowserContext(): void {
+  if ('indexedDB' in window && indexedDB !== null && navigator.cookieEnabled) {


Is the cookieEnabled a separate condition? I don't want to give a misleading error that indexedDB isn't available if it is available, but cookies are not.

hsubox76 · 2020-06-23T20:58:17Z

packages/analytics/index.ts

+
+      request.onerror = () => {
+        throw ERROR_FACTORY.create(AnalyticsError.INVALID_INDEXED_DB_CONTEXT, {
+          errorInfo: request.error!.message


any possibility there might not be an error or a message? would it be safer to go with request.error?.message || '' just in case?

hsubox76 · 2020-06-23T20:59:11Z

packages/analytics/index.ts

+    throw ERROR_FACTORY.create(AnalyticsError.INDEXED_DB_UNSUPPORTED);
+  }
+}
+function isSupported(): boolean {


Should probably have jsdoc style comment explaining what this function does.

hsubox76 · 2020-06-23T20:59:32Z

packages/analytics/index.ts

  }
  interface FirebaseApp {
    analytics(): FirebaseAnalytics;
  }
 }
+function validateBrowserContext(): void {


Should probably add jsdoc style comment explaining what this function does.

hsubox76 · 2020-06-23T21:01:09Z

packages/firebase/index.d.ts

@@ -5083,6 +5083,7 @@ declare namespace firebase.analytics {
    id?: string;
    name?: string;
  }
+  function isSupported(): boolean;


Will need a comment here to document what this does. Comments in this file are the source of our official documentation, see other methods/properties for format.

…into issue-2393-analytics

Plan to move validation functions to util.

…into issue-2393-analytics

hsubox76

This looks good, thanks for all the changes and sorry for suggesting one more. The other comments are just grammar nits - "cookies are enabled/disabled" is usually how the functionality is referred to in communication even though I know the window property is cookieEnabled.

hsubox76 · 2020-07-10T19:48:06Z

packages/analytics/index.ts

+import {
+  isIndexedDBAvailable,
+  validateIndexedDBOpenable,
+  isCookieEnabled


Nit: areCookiesEnabled

hsubox76 · 2020-07-10T19:49:29Z

packages/analytics/src/errors.ts

+  [AnalyticsError.INVALID_INDEXED_DB_CONTEXT]:
+    "Environment doesn't support IndexedDB: {$errorInfo}. Wrap initialization of analytics in analytics.isSupported() to prevent intialization in unsupported environments",
+  [AnalyticsError.COOKIE_NOT_ENABLED]:
+    'Cookie not enabled in this browser environment. Analytics requires cookies to be enabled.'


Nit: "Cookies are not"

hsubox76 · 2020-07-10T19:49:42Z

packages/analytics/src/errors.ts

+    'IndexedDB is not supported by current browswer',
+  [AnalyticsError.INVALID_INDEXED_DB_CONTEXT]:
+    "Environment doesn't support IndexedDB: {$errorInfo}. Wrap initialization of analytics in analytics.isSupported() to prevent intialization in unsupported environments",
+  [AnalyticsError.COOKIE_NOT_ENABLED]:


Nit: COOKIES_NOT_ENABLED

hsubox76 · 2020-07-10T19:51:21Z

packages/analytics/src/errors.ts

+  [AnalyticsError.INDEXED_DB_UNSUPPORTED]:
+    'IndexedDB is not supported by current browswer',
+  [AnalyticsError.INVALID_INDEXED_DB_CONTEXT]:
+    "Environment doesn't support IndexedDB: {$errorInfo}. Wrap initialization of analytics in analytics.isSupported() to prevent intialization in unsupported environments",


Line wrap by breaking up string ("" + ""), and second "initialization" (after "prevent") missing a letter.

hsubox76 · 2020-07-10T19:59:14Z

packages/analytics/src/helpers.ts

+ * This method also validates browser context for indexedDB by opening a dummy indexedDB database and throws AnalyticsError.INVALID_INDEXED_DB_CONTEXT
+ * if errors occur during the database open operation.
+ */
+export async function validateInitializationEnvironment(): Promise<void> {


First, sorry for so many changes. Figuring this functionality out is kind of uncharted territory so there's been a lot of trying things out and undoing them if they don't work.

Since there's async and sync parts I think maybe we shouldn't have a single function for this anymore.

I would suggest the first 2 if blocks in factory() just as they are here (since they can actually block initialization, why not let them?)

And then for the validateIndexedDBOpenable() check, call it with a chained .catch() where you add the analytics-specific text and re-throw the error just as you do here.

…into issue-2393-analytics

hsubox76

Looks just about ready!

One comment about maybe getting rid of eslint comment?
Get rid of 2 stray auth files? (maybe came from a merge? checkout from origin/master)
Change PR title to be descriptive of content: "Add environment check to analytics"?

hsubox76 · 2020-07-14T21:02:49Z

packages/analytics/src/factory.ts

+  }
+  if (!isIndexedDBAvailable()) {
+    throw ERROR_FACTORY.create(AnalyticsError.INDEXED_DB_UNSUPPORTED);
+  }
  // Async but non-blocking.
  // eslint-disable-next-line @typescript-eslint/no-floating-promises


Is eslint ok now if we get rid of this comment?

…into issue-2393-analytics

schmidt-sebastian and others added 8 commits June 4, 2020 12:01

updateDoc()/deleteDoc() signature fix (#3147)

0843a6e

Transaction/WriteBatch signature fix (#3151)

0c426c3

Take WriteStream offline when IndexedDB is unavailable (#2995)

fd0c0a3

Do not build firestore lite in build because it breaks regular releas…

9512b48

…e build (#3156) * Do not build firestore lite in build because it breaks release build * add release build script

add pre script for build:release (#3161)

b5c7b78

Add setLogLevel() (#3154)

e10388c

Add DocumentReference (#3123)

3ac0fe3

issue #2393 fix for analytics module

a377c68

XuechunHou added the api: analytics label Jun 4, 2020

XuechunHou requested review from egilmorez, Feiyang1 and hsubox76 as code owners June 4, 2020 18:25

XuechunHou assigned hsubox76 Jun 4, 2020

XuechunHou added 9 commits June 4, 2020 13:24

removed unnecessary sw check within isSupported method for analytics

5932e87

resolved merge conflicts

541bb0c

using raw indexDB api to open a dummy database

0090a18

Merge branch 'master' of https://github.com/firebase/firebase-js-sdk …

8e979a3

…into issue-2393-analytics

added console log for reading code

7d419f1

Merge branch 'master' of https://github.com/firebase/firebase-js-sdk …

08e4a6f

…into issue-2393-analytics

fix for issue-2393

571e514

Merge branch 'master' of https://github.com/firebase/firebase-js-sdk …

b22c1fc

…into issue-2393-analytics

removed unused import

dc75ec1

XuechunHou requested a review from hiranya911 as a code owner June 17, 2020 22:17

fixed so that correct type of variable of errorInfo required errorFac…

64d8be7

…tory is passed

hsubox76 reviewed Jun 23, 2020

View reviewed changes

XuechunHou added 4 commits June 23, 2020 15:55

Merge branch 'master' of https://github.com/firebase/firebase-js-sdk …

099816a

…into issue-2393-analytics

Merge branch 'master' of https://github.com/firebase/firebase-js-sdk …

c8cc946

…into issue-2393-analytics

fixed isSupported export

d16cbbd

addressed feedback

ad09146

hsubox76 previously approved these changes Jul 7, 2020

View reviewed changes

hsubox76 self-requested a review July 7, 2020 17:46

Merge branch 'master' of https://github.com/firebase/firebase-js-sdk …

d7b3359

…into issue-2393-analytics

XuechunHou added 6 commits July 7, 2020 14:29

bring functions to util

12852f9

convert validateIndexedDBOpenable to async

e9eb751

Merge branch 'master' of https://github.com/firebase/firebase-js-sdk …

e00edfa

…into issue-2393-analytics

trying to fix async error throwing

68387e6

brought indexedDB check to factory method

17973b6

Merge branch 'master' of https://github.com/firebase/firebase-js-sdk …

44e5aeb

…into issue-2393-analytics

hsubox76 reviewed Jul 10, 2020

View reviewed changes

XuechunHou added 4 commits July 10, 2020 15:49

fixed grammar error

1321327

break down functions

6debf5d

Merge branch 'master' of https://github.com/firebase/firebase-js-sdk …

21a4f7c

…into issue-2393-analytics

take indexedDB check funcitons out to factory method

db750f8

XuechunHou requested a review from bojeil-google as a code owner July 13, 2020 22:59

changed error names

4dec8d9

hsubox76 reviewed Jul 14, 2020

View reviewed changes

XuechunHou added 2 commits July 14, 2020 15:19

removed eslint comment

9d46212

Merge branch 'master' of https://github.com/firebase/firebase-js-sdk …

51303b0

…into issue-2393-analytics

XuechunHou changed the title ~~Issue 2393 fix - analytics module~~ Add environment check to analytics Jul 14, 2020

revert license change

64df7e5

XuechunHou changed the title ~~Add environment check to analytics~~ Issue 2393 - Add environment check to analytics Jul 14, 2020

XuechunHou changed the title ~~Issue 2393 - Add environment check to analytics~~ Issue 2393 - Add environment check to Analytics Module Jul 14, 2020

hsubox76 approved these changes Jul 16, 2020

View reviewed changes

XuechunHou merged commit 02419ce into master Jul 16, 2020

XuechunHou deleted the issue-2393-analytics branch July 16, 2020 20:43

google-oss-bot mentioned this pull request Jul 22, 2020

Version Packages #3449

Merged

hsubox76 mentioned this pull request Jul 23, 2020

Use Dynamic Measurement ID in Analytics #2800

Merged

firebase locked and limited conversation to collaborators Aug 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 2393 - Add environment check to Analytics Module #3165

Issue 2393 - Add environment check to Analytics Module #3165

XuechunHou commented Jun 4, 2020

google-oss-bot commented Jun 4, 2020 •

edited

Loading

`@firebase/analytics`

`@firebase/util`

`firebase`

hsubox76 left a comment

hsubox76 Jun 23, 2020

hsubox76 Jun 23, 2020

hsubox76 Jun 23, 2020

hsubox76 Jun 23, 2020

hsubox76 Jun 23, 2020

hsubox76 Jun 23, 2020

hsubox76 Jun 23, 2020

hsubox76 left a comment

hsubox76 Jul 10, 2020

hsubox76 Jul 10, 2020

hsubox76 Jul 10, 2020

hsubox76 Jul 10, 2020

hsubox76 Jul 10, 2020

hsubox76 left a comment

hsubox76 Jul 14, 2020

Issue 2393 - Add environment check to Analytics Module #3165

Issue 2393 - Add environment check to Analytics Module #3165

Conversation

XuechunHou commented Jun 4, 2020

google-oss-bot commented Jun 4, 2020 • edited Loading

Binary Size Report

Affected SDKs

@firebase/analytics

@firebase/util

firebase

Test Logs

hsubox76 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsubox76 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsubox76 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-oss-bot commented Jun 4, 2020 •

edited

Loading

`@firebase/analytics`

`@firebase/util`

`firebase`