fix: browser detection

[JoseVSeb]

Update browser detection logic:
Detect either window or web worker (WorkerGlobalScope).

Fixes #8299
Fixes #8284

[changeset-bot]

🦋 Changeset detected

Latest commit: 3a2a1ba

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 29 packages

Name	Type
@firebase/util	Patch
@firebase/app	Patch
@firebase/analytics-compat	Patch
@firebase/analytics	Patch
@firebase/app-check-compat	Patch
@firebase/app-check	Patch
@firebase/app-compat	Patch
@firebase/auth-compat	Patch
@firebase/auth	Patch
@firebase/component	Patch
@firebase/database-compat	Patch
@firebase/database-types	Patch
@firebase/database	Patch
firebase	Patch
@firebase/firestore-compat	Patch
@firebase/firestore	Patch
@firebase/functions-compat	Patch
@firebase/functions	Patch
@firebase/installations-compat	Patch
@firebase/installations	Patch
@firebase/messaging-compat	Patch
@firebase/messaging	Patch
@firebase/performance-compat	Patch
@firebase/performance	Patch
@firebase/remote-config-compat	Patch
@firebase/remote-config	Patch
@firebase/storage-compat	Patch
@firebase/storage	Patch
@firebase/vertexai-preview	Patch

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

[jdgamble555]

Thanks for working on this guys!

[DellaBitta]

Hi @JoseVSeb,

Unfortunately adding "WebWorker" to the list of libs breaks our build enviornment. That lib includes symbols with the same name as those within other libs but with significantly different signatures.

I'm still working on this issue, though, and trying to find alternatives to a change that requires the use of WorkerGlobalScope. Could you please see if this code properly returns when you're in a webworker?

export function isWebWorker(): boolean {
  return !isNode() && typeof window === 'undefined' && typeof self !== 'undefined';
}

isNode() is the function in the same enviornment.ts file you added edits to. Thanks!

[JoseVSeb]

Hi @DellaBitta,

export function isWebWorker(): boolean {
  return !isNode() && typeof window === 'undefined' && typeof self !== 'undefined';
}

this detects web worker fine but it also detects Edge runtime (unless forceEnvironment is set to "node", not ideal), which was the issue to begin with.

Unfortunately adding "WebWorker" to the list of libs breaks our build enviornment. That lib includes symbols with the same name as those within other libs but with significantly different signatures.

I added WebWorker to libs since I was using WorkerGlobalScope to detect web worker, but it's not technically necessary. If it's causing issues, just putting declare class WorkerGlobalScope {} in environment.ts might be enough.

I'll update the PR.

[DellaBitta]

Ok, thank you! Another change request, if you would!

The current PR adds a call to isWebWorker() to the implementation of isBrowser(), but there could be situations where this isn't quite valid. If some code wants to determine if they're in a browser, and expects all of the browser elements to be there, then this code could cause probelms because a Browser is also a WebWorker with this logic

Instead of adding || isWebWorker() to the implementation of isBrowser(), could you move the check to the implementation of initializeServerApp()?

Something like this:

export function initializeServerApp(
  _options: FirebaseOptions | FirebaseApp,
  _serverAppConfig: FirebaseServerAppSettings
): FirebaseServerApp {
  if (isBrowser() && !isWebWorker()) {
    // FirebaseServerApp isn't designed to be run in browsers.
    throw ERROR_FACTORY.create(AppError.INVALID_SERVER_APP_ENVIRONMENT);
  }
  ...

This would mean that the PR doesn't fix issue #8284 (the Auth Lite issue), but perhaps it will enable FirebaseServerApp in those environments. I can escalate that issue with the Auth team directly.

[JoseVSeb]

The current PR adds a call to isWebWorker() to the implementation of isBrowser(), but there could be situations where this isn't quite valid. If some code wants to determine if they're in a browser, and expects all of the browser elements to be there, then this code could cause probelms because a Browser is also a WebWorker with this logic

I called isWebWorker() inside isBrowser() so as not to break the intention behind the previous commit (see #1773) affecting this function as I am not entirely sure of the impact of that commit. in #1773 (comment), it refers this usage:

firebase-js-sdk/packages/app/index.ts

Line 26 in f6afa45

if (isBrowser() && 'firebase' in self) {

---

  if (isBrowser() && !isWebWorker()) {

did you mean isBrowser() || isWebWorker()? Since window and WorkerGlobalScope are never both defined in the same place, checking isBrowser() && !isWebWorker() is the same as just checking isBrowser(). also, FirebaseServerApp can technically work in a web worker and there are scenarios where that might be needed. so, in my opinion, if we are to remove isWebWorker() check from inside isBrowser(), initializeServerApp should check for just window.

---

This would mean that the PR doesn't fix issue #8284 (the Auth Lite issue), but perhaps it will enable FirebaseServerApp in those environments. I can escalate that issue with the Auth team directly.

I'm not exactly certain of that issue but #8284 (comment) seems to suggest that this could be a possible fix, that's why I mentioned it.

[DellaBitta]

I suppose it's worth mentioning some of the team's goals:

• I'm trying to get to place where initializeServerApp() is enabled for WebWorkers, at least experimentally.
• Currently initializeServerApp() throws an error for WebWorkers because isBrowser() is returning true when running in a WebWorker. We want to fix this.
• As a team we don't want FirebaseServerApp to be used in standard browser environments (ie: non-WebWorkers) so we want initializeServerApp() to still throw an error for standard browser runtimes.
• If we keep the implementation of isBrowser() as it is right now, it can be used as a tool to detect an environment that is in a browser and is NOT a web worker, ie: (isBrowser() && !isWebWorker()), which is advantageous for the SDK.
• isBrowser() is used by the Auth Compat SDK, too. I would rather not change the Auth Compat SDK's behavior (by changing the isBrowser() implementation) as that could have unknown repercussions. We can further consult the Auth team but that will take some time.
• With all of this, I think the best path forward is to add the isWebWorker() function, keep isBrowser() as is, and update initializeServerApp() so that it allows WebWorkers to proceed without throwing an error:

if (isBrowser() && !isWebWorker()) {
    // FirebaseServerApp isn't designed to be run in browsers.
    throw ERROR_FACTORY.create(AppError.INVALID_SERVER_APP_ENVIRONMENT);
}

I know this is a lot, so if you'd rather I do this this work in a new PR then let me know!

[DellaBitta]

Please revert the change to isBrowser's implementation and then we should be good to go!

[JoseVSeb]

@DellaBitta, I raised this PR because isBrowser() has issues. The existing logic uses self to detect a browser, which returns true for every runtime that has self defined (not just browser), including Next.JS Edge Runtime, which is one of the two reasons behind #8299. Edge Runtime is NOT a web worker.

I traced the source of change to #1773 where browser detection was changed from window to self and the reasoning stated as "Web Workers are browser too".

I raised this PR so that isBrowser does what it's supposed to do and also, not break #1773; explicitly detecting either window or WorkerGlobalScope (for web worker) so that it covers both the runtimes available in a browser and not detect other runtimes, which solves the first cause of #8299 (second cause is covered by #8320).

[DellaBitta]

Ah, I see! Sorry for the confusion. This looks ok to me but let me get one more pair of eyes on it.

[DellaBitta]

Merging. Thanks for the contribution!