Skip to content

Remove isomorphic-fetch dependency #3782

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Sep 21, 2020
Merged

Remove isomorphic-fetch dependency #3782

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2df6538
Remove isomorphic-fetch from functions
hsubox76 Sep 15, 2020
532d163
Change to global, globalThis needs Node 12+
hsubox76 Sep 15, 2020
bd58a1f
Update comment
hsubox76 Sep 15, 2020
fa623e0
Pass fetch implementation to functions service
hsubox76 Sep 16, 2020
fe963f8
Remove namespace exports from functions-exp
hsubox76 Sep 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions packages-exp/functions-exp/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,9 @@
"@firebase/functions-types-exp": "0.0.800",
"@firebase/messaging-types": "0.5.0",
"@firebase/util": "0.3.2",
"isomorphic-fetch": "2.2.1",
"tslib": "^1.11.1"
"node-fetch": "2.6.1",
"tslib": "^1.11.1",
"whatwg-fetch": "3.4.1"
},
"nyc": {
"extension": [
Expand Down
14 changes: 13 additions & 1 deletion packages-exp/functions-exp/src/index.node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,22 @@
*/
import { registerVersion } from '@firebase/app-exp';
import { registerFunctions } from './config';
import 'isomorphic-fetch';
import 'whatwg-fetch';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of including a new polyfill, we can replace Header with a string map. I prefer to not include a new polyfill that only provides Header and given that the workaround is really simple.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed (it is a string-keyed object, fetch signature does not accept Maps).

import nodeFetch from 'node-fetch';

import { name, version } from '../package.json';

/**
* Patch global object with node-fetch. whatwg-fetch polyfill patches other global
* fetch objects such as Headers. Then we override the implemenation of `fetch()`
* itself with node-fetch.
* https://github.com/node-fetch/node-fetch#loading-and-configuring-the-module
* node-fetch type deviates somewhat from fetch spec:
* https://github.com/apollographql/apollo-link/issues/513#issuecomment-548219023
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(global as any).fetch = (nodeFetch as unknown) as typeof fetch;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried about introducing hard to debug issues in developers' app by patching globals. For example, they might use a different fetch polyfill which can be overwritten by our node-fetch polyfill depending on the import order. node-fetch might have a bug that the other polyfill doesn't have. It will be really hard for developers to track down the issue.

Though the chance of this kind of bug is slim, but it's not impossible, I think it's a good practice to not patch globals to future proof the SDK and developers' apps.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, it is passed in now.


export * from './api';

registerFunctions();
Expand Down
14 changes: 13 additions & 1 deletion packages/functions/index.node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,21 @@
import firebase from '@firebase/app';
import { _FirebaseNamespace } from '@firebase/app-types/private';
import { registerFunctions } from './src/config';
import 'isomorphic-fetch';
import 'whatwg-fetch';
import nodeFetch from 'node-fetch';

import { name, version } from './package.json';

/**
* Patch global object with node-fetch. whatwg-fetch polyfill patches other global
* fetch objects such as Headers. Then we override the implemenation of `fetch()`
* itself with node-fetch.
* https://github.com/node-fetch/node-fetch#loading-and-configuring-the-module
* node-fetch type deviates somewhat from fetch spec:
* https://github.com/apollographql/apollo-link/issues/513#issuecomment-548219023
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(global as any).fetch = (nodeFetch as unknown) as typeof fetch;

registerFunctions(firebase as _FirebaseNamespace);
firebase.registerVersion(name, version, 'node');
11 changes: 7 additions & 4 deletions packages/functions/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
"browser": "dist/index.cjs.js",
"module": "dist/index.esm.js",
"esm2017": "dist/index.esm2017.js",
"files": ["dist"],
"files": [
"dist"
],
"scripts": {
"lint": "eslint -c .eslintrc.js '**/*.ts' --ignore-path '../../.gitignore'",
"lint:fix": "eslint --fix -c .eslintrc.js '**/*.ts' --ignore-path '../../.gitignore'",
Expand Down Expand Up @@ -45,11 +47,12 @@
},
"typings": "dist/index.d.ts",
"dependencies": {
"@firebase/component": "0.1.19",
"@firebase/functions-types": "0.3.17",
"@firebase/messaging-types": "0.5.0",
"@firebase/component": "0.1.19",
"isomorphic-fetch": "2.2.1",
"tslib": "^1.11.1"
"node-fetch": "2.6.1",
"tslib": "^1.11.1",
"whatwg-fetch": "3.4.1"
},
"nyc": {
"extension": [
Expand Down
22 changes: 2 additions & 20 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8934,7 +8934,7 @@ is-stream-ended@^0.1.4:
resolved "https://registry.npmjs.org/is-stream-ended/-/is-stream-ended-0.1.4.tgz#f50224e95e06bce0e356d440a4827cd35b267eda"
integrity sha512-xj0XPvmr7bQFTvirqnFr50o0hQIh6ZItDqloxt5aJrR4NQsYeSsyFQERYGCAzfindAcnKjINnwEEgLx4IqVzQw==

is-stream@^1.0.0, is-stream@^1.0.1, is-stream@^1.1.0:
is-stream@^1.0.0, is-stream@^1.1.0:
version "1.1.0"
resolved "https://registry.npmjs.org/is-stream/-/is-stream-1.1.0.tgz#12d4a3dd4e68e0b79ceb8dbc84173ae80d91ca44"
integrity sha1-EtSj3U5o4Lec6428hBc66A2RykQ=
Expand Down Expand Up @@ -9075,14 +9075,6 @@ isobject@^3.0.0, isobject@^3.0.1:
resolved "https://registry.npmjs.org/isobject/-/isobject-3.0.1.tgz#4e431e92b11a9731636aa1f9c8d1ccbcfdab78df"
integrity sha1-TkMekrEalzFjaqH5yNHMvP2reN8=

[email protected]:
version "2.2.1"
resolved "https://registry.npmjs.org/isomorphic-fetch/-/isomorphic-fetch-2.2.1.tgz#611ae1acf14f5e81f729507472819fe9733558a9"
integrity sha1-YRrhrPFPXoH3KVB0coGf6XM1WKk=
dependencies:
node-fetch "^1.0.1"
whatwg-fetch ">=0.10.0"

isstream@~0.1.2:
version "0.1.2"
resolved "https://registry.npmjs.org/isstream/-/isstream-0.1.2.tgz#47e63f7af55afa6f92e1500e690eb8b8529c099a"
Expand Down Expand Up @@ -11100,14 +11092,6 @@ [email protected], node-fetch@^2.2.0, node-fetch@^2.3.0, node-fetch@^2.5.0, node-
resolved "https://registry.npmjs.org/node-fetch/-/node-fetch-2.6.1.tgz#045bd323631f76ed2e2b55573394416b639a0052"
integrity sha512-V4aYg89jEoVRxRb2fJdAg8FHvI7cEyYdVAh94HH0UIK8oJxUfkjlDQN9RbMx+bEjP7+ggMiFRprSti032Oipxw==

node-fetch@^1.0.1:
version "1.7.3"
resolved "https://registry.npmjs.org/node-fetch/-/node-fetch-1.7.3.tgz#980f6f72d85211a5347c6b2bc18c5b84c3eb47ef"
integrity sha512-NhZ4CsKx7cYm2vSrBAr2PvFOe6sWDf0UYLRqA6svUYg7+/TSfVAu49jYC4BvQ4Sms9SZgdqGBgroqfDhJdTyKQ==
dependencies:
encoding "^0.1.11"
is-stream "^1.0.1"

node-forge@^0.10.0:
version "0.10.0"
resolved "https://registry.npmjs.org/node-forge/-/node-forge-0.10.0.tgz#32dea2afb3e9926f02ee5ce8794902691a676bf3"
Expand Down Expand Up @@ -14361,7 +14345,6 @@ symbol-observable@^1.1.0:

"sync-promise@git+https://github.com/brettz9/sync-promise.git#full-sync-missing-promise-features":
version "1.0.1"
uid "25845a49a00aa2d2c985a5149b97c86a1fcdc75a"
resolved "git+https://github.com/brettz9/sync-promise.git#25845a49a00aa2d2c985a5149b97c86a1fcdc75a"

table@^5.2.3:
Expand Down Expand Up @@ -15670,7 +15653,6 @@ websocket-extensions@>=0.1.1:

"websql@git+https://github.com/brettz9/node-websql.git#configurable-secure2":
version "1.0.0"
uid "5149bc0763376ca757fc32dc74345ada0467bfbb"
resolved "git+https://github.com/brettz9/node-websql.git#5149bc0763376ca757fc32dc74345ada0467bfbb"
dependencies:
argsarray "^0.0.1"
Expand All @@ -15684,7 +15666,7 @@ [email protected]:
resolved "https://registry.npmjs.org/whatwg-fetch/-/whatwg-fetch-2.0.4.tgz#dde6a5df315f9d39991aa17621853d720b85566f"
integrity sha512-dcQ1GWpOD/eEQ97k66aiEVpNnapVj90/+R+SXTPYGHpYBBypfKJEQjLrvMZ7YXbKm21gXd4NcuxUTjiv1YtLng==

whatwg-fetch@>=0.10.0:
whatwg-fetch@3.4.1:
version "3.4.1"
resolved "https://registry.npmjs.org/whatwg-fetch/-/whatwg-fetch-3.4.1.tgz#e5f871572d6879663fa5674c8f833f15a8425ab3"
integrity sha512-sofZVzE1wKwO+EYPbWfiwzaKovWiZXf4coEzjGP9b2GBVgQRLQUZ2QcuPpQExGDAW5GItpEm6Tl4OU5mywnAoQ==
Expand Down