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

fix: upgrade to node-fetch@3.X MONGOSH-1702 #1845

Merged
merged 23 commits into from
Mar 6, 2024
Merged

fix: upgrade to node-fetch@3.X MONGOSH-1702 #1845

merged 23 commits into from
Mar 6, 2024

Conversation

paula-stacho
Copy link
Contributor

@paula-stacho paula-stacho commented Feb 29, 2024

https://jira.mongodb.org/browse/MONGOSH-1702

Description

The new version doesn't work with our tsconfig, that's why the "skipLibCheck". I hope it's okay I added it to the common config, I've seen we already have it in @mongodb-js/tsconfig-devtools

Code changes are due to:

  1. response.body can be null - this was probably already true before, just the types were weaker. the upgrade guide only mentions removing a string option. from what I see we've been treating it as a stream. https://github.com/node-fetch/node-fetch/blob/main/docs/v3-UPGRADE-GUIDE.md#reqbody-can-no-longer-be-a-string
  2. response.json() is Promise<unknown>. again, the types were probably just weaker before and it passed as any.
  3. node-fetch is now an ES module https://github.com/node-fetch/node-fetch/blob/main/docs/v3-UPGRADE-GUIDE.md#converted-to-es-module -> this one created considerable issues in different builds and per @addaleax's suggestion led to the creation of the import-node-fetch wrapper
  4. once is now recommended when listening for errors https://github.com/node-fetch/node-fetch/blob/main/docs/v3-UPGRADE-GUIDE.md#a-stream-pipeline-is-now-used-to-forward-errors

Note: You'll see the code updates in the build package as well, but not the version change. Originally I thought I might as well upgrade node-fetch everywhere, but the ES Module doesn't work with ts-node. Looks like ESM support in ts-node comes with a warning, and this isn't directly connected to the deprecation warning in homebrew, so I think we can leave this on 2.x for now.

packages/snippet-manager/package.json Outdated Show resolved Hide resolved
packages/cli-repl/src/update-notification-manager.ts Outdated Show resolved Hide resolved
packages/e2e-tests/test/fixtures/curl.js Show resolved Hide resolved
packages/cli-repl/src/update-notification-manager.ts Outdated Show resolved Hide resolved
@paula-stacho paula-stacho force-pushed the MONGOSH-1702 branch 2 times, most recently from f8476c7 to b2ef18d Compare March 4, 2024 12:34
packages/import-node-fetch/src/index.ts Outdated Show resolved Hide resolved
packages/import-node-fetch/package.json Outdated Show resolved Hide resolved
packages/import-node-fetch/src/index.spec.ts Outdated Show resolved Hide resolved
@paula-stacho paula-stacho force-pushed the MONGOSH-1702 branch 2 times, most recently from b741f1b to b900d03 Compare March 4, 2024 17:02
@paula-stacho paula-stacho marked this pull request as ready for review March 5, 2024 12:40
@paula-stacho paula-stacho merged commit 19f117c into main Mar 6, 2024
55 of 59 checks passed
@paula-stacho paula-stacho deleted the MONGOSH-1702 branch March 6, 2024 14:22
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 this pull request may close these issues.

2 participants