-
Notifications
You must be signed in to change notification settings - Fork 928
Issue 2393 - Add environment check to Analytics Module #3165
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 39 commits
0843a6e
0c426c3
fd0c0a3
9512b48
b5c7b78
e10388c
3ac0fe3
a377c68
5932e87
541bb0c
0090a18
8e979a3
7d419f1
08e4a6f
571e514
b22c1fc
dc75ec1
64d8be7
099816a
c8cc946
d16cbbd
ad09146
948a5da
a1103e1
f341f94
111d792
c9f137b
1fa549c
9d12700
2b00fc1
c5e2596
e30d0ec
d7b3359
12852f9
e9eb751
e00edfa
68387e6
17973b6
44e5aeb
1321327
6debf5d
21a4f7c
db750f8
4dec8d9
9d46212
51303b0
64df7e5
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,9 @@ | ||
--- | ||
"@firebase/analytics": minor | ||
"firebase": minor | ||
--- | ||
|
||
Issue 2393 fix - analytics module | ||
|
||
- Added a public method `isSupported` to Analytics module which returns true if current browser context supports initialization of analytics module. | ||
- Added runtime checks to Analytics module that validate if cookie is enabled in current browser and if current browser environment supports indexedDB functionalities. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,4 +56,4 @@ | |
], | ||
"reportDir": "./coverage/node" | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,10 @@ export const enum AnalyticsError { | |
NO_GA_ID = 'no-ga-id', | ||
ALREADY_EXISTS = 'already-exists', | ||
ALREADY_INITIALIZED = 'already-initialized', | ||
INTEROP_COMPONENT_REG_FAILED = 'interop-component-reg-failed' | ||
INTEROP_COMPONENT_REG_FAILED = 'interop-component-reg-failed', | ||
INDEXED_DB_UNSUPPORTED = 'indexedDB-unsupported', | ||
INVALID_INDEXED_DB_CONTEXT = 'invalid-indexedDB-context', | ||
COOKIE_NOT_ENABLED = 'cookie-not-enabled' | ||
} | ||
|
||
const ERRORS: ErrorMap<AnalyticsError> = { | ||
|
@@ -39,12 +42,19 @@ const ERRORS: ErrorMap<AnalyticsError> = { | |
'settings() must be called before initializing any Analytics instance' + | ||
'or it will have no effect.', | ||
[AnalyticsError.INTEROP_COMPONENT_REG_FAILED]: | ||
'Firebase Analytics Interop Component failed to instantiate' | ||
'Firebase Analytics Interop Component failed to instantiate', | ||
[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", | ||
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. Line wrap by breaking up string ("" + ""), and second "initialization" (after "prevent") missing a letter. |
||
[AnalyticsError.COOKIE_NOT_ENABLED]: | ||
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. Nit: COOKIES_NOT_ENABLED |
||
'Cookie not enabled in this browser environment. Analytics requires cookies to be enabled.' | ||
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. Nit: "Cookies are not" |
||
}; | ||
|
||
interface ErrorParams { | ||
[AnalyticsError.ALREADY_EXISTS]: { id: string }; | ||
[AnalyticsError.INTEROP_COMPONENT_REG_FAILED]: { reason: Error }; | ||
[AnalyticsError.INVALID_INDEXED_DB_CONTEXT]: { errorInfo: string }; | ||
} | ||
|
||
export const ERROR_FACTORY = new ErrorFactory<AnalyticsError, ErrorParams>( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,7 +32,8 @@ import { | |
insertScriptTag, | ||
getOrCreateDataLayer, | ||
wrapOrCreateGtag, | ||
findGtagScriptOnPage | ||
findGtagScriptOnPage, | ||
validateInitializationEnvironment | ||
} from './helpers'; | ||
import { ANALYTICS_ID_FIELD } from './constants'; | ||
import { AnalyticsError, ERROR_FACTORY } from './errors'; | ||
|
@@ -117,6 +118,10 @@ export function factory( | |
app: FirebaseApp, | ||
installations: FirebaseInstallations | ||
): FirebaseAnalytics { | ||
// Async but non-blocking. | ||
// eslint-disable-next-line @typescript-eslint/no-floating-promises | ||
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. Is eslint ok now if we get rid of this comment? |
||
validateInitializationEnvironment(); | ||
|
||
const analyticsId = app.options[ANALYTICS_ID_FIELD]; | ||
if (!analyticsId) { | ||
throw ERROR_FACTORY.create(AnalyticsError.NO_GA_ID); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,7 +32,12 @@ import { | |
} from './constants'; | ||
import { FirebaseInstallations } from '@firebase/installations-types'; | ||
import { logger } from './logger'; | ||
|
||
import { ERROR_FACTORY, AnalyticsError } from './errors'; | ||
import { | ||
isIndexedDBAvailable, | ||
validateIndexedDBOpenable, | ||
isCookieEnabled | ||
} from '@firebase/util'; | ||
/** | ||
* Initialize the analytics instance in gtag.js by calling config command with fid. | ||
* | ||
|
@@ -218,3 +223,27 @@ export function findGtagScriptOnPage(): HTMLScriptElement | null { | |
} | ||
return null; | ||
} | ||
|
||
/** | ||
* This method checks whether cookie is enabled within current browser and throws AnalyticsError.COOKIE_NOT_ENABLED if not. | ||
* | ||
* This method also checks if indexedDB is supported by current browser and throws AnalyticsError.INDEXED_DB_UNSUPPORTED error if not. | ||
* | ||
* 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> { | ||
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. 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.
|
||
if (!isCookieEnabled()) { | ||
throw ERROR_FACTORY.create(AnalyticsError.COOKIE_NOT_ENABLED); | ||
} | ||
if (!isIndexedDBAvailable()) { | ||
throw ERROR_FACTORY.create(AnalyticsError.INDEXED_DB_UNSUPPORTED); | ||
} | ||
try { | ||
await validateIndexedDBOpenable(); | ||
} catch (error) { | ||
throw ERROR_FACTORY.create(AnalyticsError.INVALID_INDEXED_DB_CONTEXT, { | ||
errorInfo: error | ||
}); | ||
} | ||
} |
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.
Nit:
areCookiesEnabled