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

Use node-fetch instead of request for default transport #40

Merged
merged 69 commits into from
Sep 22, 2023

Conversation

jamierbower
Copy link
Contributor

@jamierbower jamierbower commented Sep 5, 2023

Description

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@jamierbower jamierbower requested a review from a team as a code owner September 5, 2023 10:03
@jasonwilliams
Copy link

I still get notifications about this 😂

Anyway, looks like a lot of changes, including bumping the minimum version of Node to 18. Any reason you wouldn’t do a 4.0.0 release? This all looks too much to be a patch bump.

The public API may not have changed but there may be unexpected dependencies on requests behaviours (especially after years of use).

As you’re forcing v18 of NodeJS you could use the built in fetch API, but I can understand if you’re put off by the “experimental” tag. My understanding is it’s pretty stable to use and reduces your dependencies.

Just my 2 pence.

@jamierbower jamierbower reopened this Sep 14, 2023
@jamierbower jamierbower merged commit 1f6c795 into master Sep 22, 2023
@simongregory simongregory deleted the modernise branch November 10, 2023 08:17
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.

7 participants