Remove node-fetch

[brianjenkins94]

This may eventually address #31.

[mistval]

This looks well on its way. I realized the instructions in the README for running tests was out of date, so I just pushed a change for that, but it sounds like you might have already figured out how to run them.

[brianjenkins94]

Actually, I think the "Has a url property" and "Has a redirected property" tests might be node-fetch-specific.

So I think this is ready for review.

A couple of things that I removed:

• .buffer as a method on the Response (legacy, use .arrayBuffer instead)
• NodeJS.Buffer usage anywhere (prefer browser global window.Buffer)
• fs.ReadStream support (this could be put back, I was just already wrestling with NodeJS.ReadableStream vs. Web Streams API ReadableStream types and that was about all my brain could handle; the related tests were commented-out)
• I switched the md5 hash to a sha1 hash that uses the browser global window.crypto (I would have thought Web Crypto had md5 but it doesn't)

The motivation for these changes was to gently push towards web standards and maybe a version that could work in the browser (cacache may make that difficult/impossible). Interop with the cacheable ecosystem would be cool though.

The url, redirected and size property may no longer be populated on the Response object after removing node-fetch.

I was wrong, url and redirected should be there. The size property is definitely gone though.

[mistval]

Thanks for your great work on this. I'd like to merge it after we review. It would be a major version increment so we would need an upgrade guide also but I could write that. It probably wouldn't be comprehensive since it will be impossible to know every difference between node-fetch and native fetch, but the changes to tests will be a good foundation for knowing the major differences.

Native fetch Response does seem to have url and redirected properties, I can see them printed when I run await fetch('http://google.com') in the node v22 REPL. We may just need to assign those properties to this explicitly in the NFCResponse constructor.

[brianjenkins94]

I'm not sure where to make the change for the url and redirected properties.

[mistval]

I'm not sure where to make the change for the url and redirected properties.

I pushed a change to your branch for it. The properties weren't writable like I was hoping (they were getters) so I used Object.defineProperty() and overrode clone() so that they would get copied over if you clone the response (might be nice to have clone() return an NFCResponse actually, but that's probably one for another day).

All tests are passing now! I will try to take a closer look Sunday or Monday and do an upgrade guide. I might have some more comments but I think this is likely at least 95% of the way there.

[brianjenkins94]

I was trying something with:

public override url;

constructor(/* ... */) {
  super(/* ... */)
  this.url = metaData.url;
}

that had worked for url, but hadn't quite worked for redirected, which might be marginally cleaner.

All tests are passing now! I will try to take a closer look Sunday or Monday and do an upgrade guide.

Thanks! This was fun (definitely more challenging than I had originally thought)!

This will be my first real PR to an open-source project 🎉

[brianjenkins94]

You may also be able to replace formdata-node with the FormData browser global.

[brianjenkins94]

I can also commit my tsup.config.json to help with building. Handling import("stream/web") is quite a pain.

[mistval]

This won't quite work because it doesn't clone the body stream, so if you clone a response and then try to read the bodies of both the original and the clone, it will error. I just pushed up a test for that, currently failing.

However the more I think about it, I do think it's worthwhile to try to find a solution that returns an NFCResponse here from clone(). This approach appears to work:

    _url = '';
    _redirected: boolean = false

    override get url() {
      return this._url;
    }

    override get redirected() {
      return this._redirected;
    }

    constructor(
      bodyStream: ReadableStream,
      public metaData: Omit<ResponseInit, 'headers'> & {
        url: string;
        redirected: boolean;
        counter: number;
        headers: Record<string, string[]>;
      },
      public ejectFromCache: () => Promise<unknown>,
      public returnedFromCache: boolean,
      public isCacheMiss = false,
    ) {
      super(
        bodyStream,
        metaData as any
      );

      this._url = metaData.url;
      this._redirected = metaData.redirected
    }

    public override clone(): NFCResponse {
      const superClone = super.clone() as NFCResponse;

      Object.setPrototypeOf(superClone, NFCResponse.prototype);

      superClone._url = this.url;
      superClone._redirected = this.redirected;
      superClone.ejectFromCache = this.ejectFromCache;
      superClone.returnedFromCache = this.returnedFromCache;
      superClone.isCacheMiss = this.isCacheMiss;

      return superClone;
    }

It would also be possible to replace the createNFCResponseClass() function with just a normal class export. That hackery was only needed to support CommonJS using dynamic imports from node-fetch.

[mistval]

This may be preferable:

public override clone(): NFCResponse {
      const clonedBody = super.clone();
    
      return new NFCResponse(
        clonedBody.body as ReadableStream,
        {
          url: this.url,
          redirected: this.redirected,
          status: this.status,
          statusText: this.statusText,
          headers: NFCResponse.serializeHeaders(this),
          counter: this.metaData.counter,
        },
        this.ejectFromCache,
        this.returnedFromCache,
        this.isCacheMiss,
      );
    }

Due to the caveats about changing an object's prototype.

[brianjenkins94]

Definitely reads better than Object.defineProperty().

[mistval]

I can also commit my tsup.config.json to help with building. Handling import("stream/web") is quite a pain.

When would this be needed? It seems to be building okay without that, when I run npm run tsc && npm run buildcjs.

[brianjenkins94]

I think in the output you'll find import { ReadableStream } from "stream/web"; which would fail if run in the browser.

Although, now that I think about it, you can just replace all instances of ReadableStream with globalThis.ReadableStream and continue to use your existing build process.

[mistval]

I think in the output you'll find import { ReadableStream } from "stream/web"; which would fail if run in the browser.

Ah maybe this module can support browser someday, but for now it's not necessary.