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

feat(fetch): use undici + ProxyAgent #1000

Merged
merged 11 commits into from
May 7, 2024
Merged

Conversation

kanadgupta
Copy link
Member

@kanadgupta kanadgupta commented May 6, 2024

🚥 Resolves RM-8008, RM-9687

🧰 Changes

  • Removes node-fetch in favor of its native Node.js counterpart
  • Removes formdata-node in favor of its native Node.js counterpart
  • Uses undici's ProxyAgent for proxying requests1
  • Now that we installed the latest nock beta in chore(deps): nock@beta, [email protected] #999, msw is no longer required and has been removed.
  • Removed a few useless delayConnection statements so now tests are like 2-3x faster

🧬 QA & Testing

Do tests still pass?

Footnotes

  1. We very well might have been doing proxying incorrectly this entire time? Ideally we should use the new EnvHttpProxyAgent API instead but that's only supported in undici@6 😔

@kanadgupta kanadgupta changed the title chore(deps): remove node-fetch, formdata-node, msw feat(fetch): use undici + ProxyAgent May 7, 2024
@kanadgupta kanadgupta added enhancement New feature or request refactor Issues about tackling technical debt labels May 7, 2024
@kanadgupta kanadgupta requested a review from erunion May 7, 2024 15:34
@kanadgupta kanadgupta marked this pull request as ready for review May 7, 2024 15:35
@@ -135,7 +133,6 @@ describe('rdme openapi', () => {
.basicAuth({ user: key })
.reply(200, [{ _id: 'spec1', title: 'spec1_title' }])
.post('/api/v1/api-specification', { registryUUID })
.delayConnection(1000)
Copy link
Member

Choose a reason for hiding this comment

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

love to see it


server.use(
Copy link
Member

Choose a reason for hiding this comment

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

msw i think you're really neat but my dude... the code bloat

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah... once nock started using the same underlying interceptor, it was game over

"gray-matter": "^4.0.1",
"ignore": "^5.2.0",
"mime-types": "^2.1.35",
"node-fetch": "^3.3.2",
Copy link
Member

Choose a reason for hiding this comment

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

...options,
headers,
dispatcher: proxy ? new ProxyAgent(proxy) : undefined,
Copy link
Member

Choose a reason for hiding this comment

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

lol @ the way node-fetch forced us to handle proxies 🤡

Copy link
Member Author

Choose a reason for hiding this comment

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

lol that was not exactly a node-fetch issue and more of a me issue 😭

@kanadgupta kanadgupta merged commit 9da7132 into next May 7, 2024
11 checks passed
@kanadgupta kanadgupta deleted the kanad-2024-05-06/remove-msw branch May 7, 2024 15:53
kanadgupta pushed a commit that referenced this pull request May 7, 2024
# [9.0.0-next.15](v9.0.0-next.14...v9.0.0-next.15) (2024-05-07)

### Features

* **fetch:** use undici + `ProxyAgent`  ([#1000](#1000)) ([9da7132](9da7132)), closes [#999](#999)

[skip ci]
@kanadgupta kanadgupta mentioned this pull request May 7, 2024
23 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refactor Issues about tackling technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants