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

Remove node-fetch #190

Merged
merged 1 commit into from
Feb 10, 2023
Merged

Remove node-fetch #190

merged 1 commit into from
Feb 10, 2023

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Feb 9, 2023

The package node-fetch has been removed, instead requiring that fetch be provided as an option for the fetch middleware.

This might reduce our bundle size (depending on whether our bundler is smart enough to omit this package already; I'm not sure). It also eliminates a dependency that has been a source of security advisories in the past.

We will probably want to add this or isomorphic-fetch back as a devDependency when we add tests for this middleware, but so far this middleware is not tested.

@Gudahtt Gudahtt changed the title Remove node-fetch. Remove node-fetch Feb 9, 2023
@Gudahtt Gudahtt marked this pull request as ready for review February 10, 2023 00:01
@Gudahtt Gudahtt requested a review from a team as a code owner February 10, 2023 00:01
@Gudahtt Gudahtt mentioned this pull request Feb 10, 2023
The package `node-fetch` has been removed, instead requiring that `fetch` be
provided as an option for the fetch middleware.

This might reduce our bundle size (depending on whether our bundler is smart
enough to omit this package already; I'm not sure). It also eliminates a
dependency that has been a source of security advisories in the past.

We will probably want to add it back as a `devDependency` when we add tests
for this middleware, but so far this middleware is not tested.
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

LGTM.

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