Functions: Bind passed fetch function

[hsubox76]

Fix a bug caused by PR #3782 to replace isomorphic-fetch dependency in functions. Bug occurs in Chrome extensions and possibly other environments. When passing fetch function as an argument it loses window context. Binding self so that it will work in service workers as well.

See https://stackoverflow.com/questions/44720448/fetch-typeerror-failed-to-execute-fetch-on-window-illegal-invocation for a similar issue.

Fixes #3849

[changeset-bot]

🦋 Changeset detected

Latest commit: 299ed28

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

This PR includes changesets to release 7 packages

Name	Type
@firebase/functions	Patch
firebase	Patch
@firebase/rules-unit-testing	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

[google-oss-bot]

Binary Size Report

Affected SDKs

• @firebase/analytics

  Type	Base (01bb65b)	Head (f16e945)	Diff
  esm2017	18.4 kB	18.6 kB	+224 B (+1.2%)
  main	23.3 kB	23.8 kB	+467 B (+2.0%)
  module	22.8 kB	23.3 kB	+455 B (+2.0%)

• @firebase/firestore

  Type	Base (01bb65b)	Head (f16e945)	Diff
  browser	249 kB	249 kB	+77 B (+0.0%)
  esm2017	196 kB	196 kB	+78 B (+0.0%)
  main	483 kB	484 kB	+165 B (+0.0%)
  module	247 kB	247 kB	+77 B (+0.0%)
  react-native	196 kB	197 kB	+78 B (+0.0%)

• @firebase/firestore/exp

  Type	Base (01bb65b)	Head (f16e945)	Diff
  browser	189 kB	189 kB	+78 B (+0.0%)
  main	477 kB	477 kB	+169 B (+0.0%)
  module	189 kB	189 kB	+78 B (+0.0%)
  react-native	189 kB	189 kB	+78 B (+0.0%)

• @firebase/firestore/lite

  Type	Base (01bb65b)	Head (f16e945)	Diff
  main	140 kB	140 kB	+6 B (+0.0%)

• @firebase/functions

  Type	Base (01bb65b)	Head (f16e945)	Diff
  browser	10.0 kB	10.0 kB	+11 B (+0.1%)
  esm2017	7.59 kB	7.60 kB	+11 B (+0.1%)
  module	9.77 kB	9.78 kB	+11 B (+0.1%)

• firebase

  Type	Base (01bb65b)	Head (f16e945)	Diff
  firebase-analytics.js	35.8 kB	35.9 kB	+70 B (+0.2%)
  firebase-firestore.js	287 kB	287 kB	+77 B (+0.0%)
  firebase-functions.js	10.1 kB	10.1 kB	+11 B (+0.1%)
  firebase.js	831 kB	831 kB	+169 B (+0.0%)

Test Logs

• Base (01bb65b5): https://github.com/firebase/firebase-js-sdk/actions/runs/277057132
• Head (f16e9456): https://github.com/firebase/firebase-js-sdk/actions/runs/282640660

[google-oss-bot]

Size Analysis Report

Affected Products

@firebase/functions-exp

• getFunctions

  Size

  Type	Base (01bb65b)	Head (f16e945)	Diff
  size	1.98 kB	1.99 kB	+11 B (+0.6%)
  size_with_ext_deps	5.86 kB	5.87 kB	+11 B (+0.2%)

• httpsCallable

  Size

  Type	Base (01bb65b)	Head (f16e945)	Diff
  size	5.33 kB	5.34 kB	+11 B (+0.2%)
  size_with_ext_deps	9.92 kB	9.93 kB	+11 B (+0.1%)

• useFunctionsEmulator

  Size

  Type	Base (01bb65b)	Head (f16e945)	Diff
  size	1.94 kB	1.95 kB	+11 B (+0.6%)
  size_with_ext_deps	5.79 kB	5.80 kB	+11 B (+0.2%)

@firebase/performance-exp

• getPerformance

  Size

  Type	Base (01bb65b)	Head (f16e945)	Diff
  size	16.8 kB	16.8 kB	+36 B (+0.2%)
  size_with_ext_deps	21.8 kB	33.8 kB	+12.0 kB (+54.9%)

• trace

  Size

  Type	Base (01bb65b)	Head (f16e945)	Diff
  size	16.7 kB	16.7 kB	+36 B (+0.2%)
  size_with_ext_deps	21.7 kB	33.8 kB	+12.1 kB (+55.5%)

Test Logs

• Base (01bb65b5): https://github.com/firebase/firebase-js-sdk/actions/runs/277057132
• Head (f16e9456): https://github.com/firebase/firebase-js-sdk/actions/runs/282640660

[Feiyang1]

Can you please add changeset?

[hsubox76]

Can you please add changeset?

Changeset added!

[Feiyang1]

Please remove @firebase/functions-exp. Exp packages should not be included in changeset.

[hsubox76]

Done.

[github-actions]

Changeset File Check ⚠️

Warning: This PR modifies files in the following packages but they have not been included in the changeset file:

• @firebase/functions-exp

Make sure this was intentional.

[zaripych]

@hsubox76 +1 for fixing the security issue with node-fetch earlier, just a bit of relevant information here that the bug actually used to happen not just in Chrome extensions. I have a CRA app (built with webpack) and was hitting this one. It was a little tricky to diagnose because exception message was hidden in here:

https://github.com/firebase/firebase-js-sdk/blob/master/packages/functions/src/api/service.ts#L200

I managed to dig-out the error to be "TypeError: Failed to execute 'fetch' on 'Window': Illegal invocation".

Anyway, with new firebase package release the issue is fixed. Just a little feedback - it would probably be great if that e variable is somehow "converted" to a proper error JSON object so that information is not lost and it's easy to diagnose what is happening.

Thanks!

[hsubox76]

Thanks for the additional information that it isn't just Chrome extensions.

As for throwing the error instead of hiding it, I did look into that, but it's tricky. Most of the time, the error that comes from the try/catch around the "fetch" is simply "Failed to fetch" (on Chrome) which will happen if it's a network error, but also if there is an error in the actual Cloud Function code, which would be misleading and cause developers to waste time trying to debug network issues instead of checking the function code. So when it was originally coded, the decision was made to hide the error because it was potentially more misleading than helpful.

If there was a way to detect and hide "Failed to fetch" errors, we could throw all other errors. However it seems brittle to detect it based on message text, which could change, and the type is TypeError, same as the one we would want to show, above. Additionally, on Firefox the error text is "NetworkError when attempting to fetch resource" and who knows what it is in other browsers.