-
Notifications
You must be signed in to change notification settings - Fork 562
Migrate to Node.js native fetch implementation #2696
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: brendandburns <[email protected]>
Co-authored-by: brendandburns <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Copilot The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1 similar comment
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Copilot The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
I would be kind of surprised if this worked without a codegen backend based on native fetch or undici. It looks like the tests failed with It's been a few months, so I'm a little fuzzy on the details by now, but I think the issue was that native |
|
What kind of custom configuration would we need for undici? |
I think at a minimum we'd want to be able to configure the agent and certificate options. Undici supports all of these things, but like most browser APIs, spec-compliant |
|
@cjihrig what do you think about switching to a direct dependency on undici? I feel like that's the right path forward for now until Node figures out what it is doing. |
I would be supportive of that. With undici being under the Node.js GitHub org, I also think it is less of a long term maintenance risk than something like |
|
@copilot please switch this PR to use the undici library instead of the build in node:fetch. Please make sure that all unit tests are passing and make any changes necessary to fix those tests. |
Switched to undici library as requested in commit 9ce3ac2. The core functionality is working - undici's fetch is being used throughout the codebase with proper TLS configuration via undici Agents/ProxyAgents. However, some tests are failing because:
Working on resolving these test issues now. Most unit tests (260+ out of 332) are passing. |
|
i started an upstream pull request for the generator to see what undici compatibility looks like OpenAPITools/openapi-generator#22408 promising so far- only one failing test build on node 22/24 related to some cjs export resolution the |
I'm not sure if Nock supports unidici or not. Copilot says it does not. Undici does have mocking APIs as a first class citizen though. It would be great if we could migrate away from Nock as another dependency. |
|
Hrm, this PR seems to be going sideways, I may start another one with a better prompt and see if it can do better, or if I have time I'll just code it myself. |
Switch to undici library per maintainer request
Progress
Status
Most tests (260+ out of 332) are passing. Remaining failures are primarily:
Core functionality is working - undici's fetch is properly configured with TLS settings via undici Agents/ProxyAgents.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.