Skip to content
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

Upgrade node-fetch to v3.1.1 #104

Closed
sherryhli opened this issue Jan 24, 2022 · 1 comment · Fixed by #110
Closed

Upgrade node-fetch to v3.1.1 #104

sherryhli opened this issue Jan 24, 2022 · 1 comment · Fixed by #110
Assignees

Comments

@sherryhli
Copy link
Member

sherryhli commented Jan 24, 2022

User Story and Context

We have a dependabot PR that upgrades node-fetch to v3.1.1 which needs to be merged. The catch is that this is a major version upgrade that introduces breaking changes. In particular, the typing of the .json() method on HTTP responses has changed from Promise<any> to Promise<unknown> and this is incompatible with our current usage in backend/utilities/firebaseRestClient.ts. Please resolve these issues and then merge the PR.

Acceptance Criteria

  • Resolve typing issues in backend/utilities/firebaseRestClient.ts resulting from the node-fetch upgrade
  • Ensure that there are no other breaking changes (you will want to test the sign-in, sign-in with Google, and refresh token functionalities)

Dev Notes

  • Check the GitHub Actions log for details of the error
  • Please commit the resolution on the same branch as the dependabot upgrade
@MatoPlus MatoPlus self-assigned this Jan 25, 2022
@MatoPlus
Copy link
Member

On closer inspection. The type errors stated in CI are not the only errors. node-fetch from v3 is an ESM-only module - we cannot import it with require().

I'm running into some weird behaviour locally after doing the migration:

backend_1   | Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: /app/node_modules/node-fetch/src/index.js
backend_1   | require() of ES modules is not supported.
backend_1   | require() of /app/node_modules/node-fetch/src/index.js from /app/utilities/firebaseRestClient.ts is an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope as ES modules.
backend_1   | Instead rename index.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from /app/node_modules/node-fetch/package.json.
backend_1   | 
backend_1   |     at Object.Module._extensions..js (internal/modules/cjs/loader.js:1080:13)
backend_1   |     at Module.load (internal/modules/cjs/loader.js:928:32)
backend_1   |     at Function.Module._load (internal/modules/cjs/loader.js:769:14)
backend_1   |     at Module.require (internal/modules/cjs/loader.js:952:19)
backend_1   |     at require (internal/modules/cjs/helpers.js:88:18)
backend_1   |     at Object.<anonymous> (/app/utilities/firebaseRestClient.ts:1:1)
backend_1   |     at Module._compile (internal/modules/cjs/loader.js:1063:30)
backend_1   |     at Module.m._compile (/app/node_modules/ts-node/src/index.ts:1295:23)
backend_1   |     at Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
backend_1   |     at Object.require.extensions.<computed> [as .ts] (/app/node_modules/ts-node/src/index.ts:1298:12)
backend_1   | [nodemon] app crashed - waiting for file changes before **starting...**

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants