Skip to content

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Mar 27, 2025

Refs: #2306

A few notes:

  • This unblocks the nock update. In fact, I believe it necessitates the update because the older nock version didn't seem to support native fetch.
  • There is future room for improvement related to the use of web streams. Native fetch uses web streams, while node-fetch uses Node streams. Just to get this migration going, I used a conversion function (search the diff for Readable.fromWeb()), but ideally we would try working with web streams directly to reduce some overhead.
  • node-fetch is still used in src/gen/http/isomorphic-fetch.ts, which I think would need to be resolved in the code generator.
  • In src/util.ts, there is a normalizeResponseHeaders() that apparently exists due to a node-fetch quirk. I did not look into whether or not that is still needed, but keeping it doesn't seem to break anything.
  • FetchError does not appear to be a thing in native fetch.

We may or may not want to keep this as a draft PR because of this list and pending the CI - what do others think?

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 27, 2025
@mstruebing
Copy link
Member

That's pretty cool already!

I don't think Readable.fromWeb() is that bad actually, sure a little bit of overhead but nothing serious.
I had a quick look into the code gen but I'm a bit overwhelmed right now.
Not sure if we can adjust the current gen code or if we would need a different/new one, as we are using a different fetching library.

@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 28, 2025

My guess is that we'd need a different code generator (or at least new capabilities in the existing one) since there are enough small, subtle differences between node-fetch and native fetch.

@brendandburns
Copy link
Contributor

brendandburns commented Mar 28, 2025

Looks good to me, my only question is for the package-lock.json there's a lot of additional dependencies that seem kind of random. Is that all really from the nock upgrade?

@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 28, 2025

Is that all really from the nock upgrade?

As far as I can tell, yes. The joys of depending on things from npm. nock started depending on @mswjs/interceptors (this is the thing that broke our tests), which brings in its own dependencies, like jsdom. You can verify any of them like this:

$ npm ls @asamuzakjp/css-color
@kubernetes/[email protected] /Users/cjihrig/programming/javascript
└─┬ [email protected]
  └─┬ @mswjs/[email protected]
    └─┬ [email protected]
      └─┬ [email protected]
        └── @asamuzakjp/[email protected]

@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 28, 2025

You can also check out this dependabot PR from last night that tried to make the same update - #2337.

@brendandburns
Copy link
Contributor

/lgtm
/approve

Thanks.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 29, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, cjihrig

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [brendandburns,cjihrig]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 815c49b into kubernetes-client:main Mar 29, 2025
8 checks passed
@cjihrig cjihrig deleted the native-fetch branch March 30, 2025 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants