chore(deps): drop node-fetch dependency

[fregante]

• Part of Dependency cleanup #3216

The fetch globals are available unflagged in v18+

[fregante]

The tests would fail when the mock Response was instantiated with <100 responses instead of throwing in the client. So I left node-fetch as a dev dependency.

[rpl]

Thanks for mentioning what corner case was still not working by just using the nodejs native Response constructor ❤️

What if instead we define a BadResponse class that extends the native Response and overrides the status property with a getter? something like:

// Used to test responses with status < 100 (nodejs native constructor
// enforces status to be in the 200-599 range and throws if it is not).
class BadResponse extends Response {
  constructor(data, fakeStatus) {
    super(data);
    this.fakeStatus = fakeStatus;
  }

  get status() {
    return this.fakeStatus;
  }
}
...
    stubMatcher.onCall(i).callsFake(async () => {
      if (status < 200) {
        return new BadResponse(body, status);
      }
      if (typeof body === 'string') {
        return new Response(body, { status });
      }
      return new JSONResponse(body, status);
    });
...

That along with removing use of File (which in the end we should be doing anyway given that the actual fileFromSync method being mocked is now returning a Blob instance) may let us remove node-fetch also from the devDependencies.

[fregante]

me when I paste code from rpl-GPT and it just works

[rpl]

@fregante on this PR there are a couple of additional changes to consider:

• one is an actual issue (due to the fact that the AMO server API validating the file extension on the form-data upload filename)
• the other one is an idea to get rid of the remaining use of node-fetch on the test side.

Let me know wdyt.

[rpl]

Thanks for mentioning what corner case was still not working by just using the nodejs native Response constructor ❤️

What if instead we define a BadResponse class that extends the native Response and overrides the status property with a getter? something like:

// Used to test responses with status < 100 (nodejs native constructor
// enforces status to be in the 200-599 range and throws if it is not).
class BadResponse extends Response {
  constructor(data, fakeStatus) {
    super(data);
    this.fakeStatus = fakeStatus;
  }

  get status() {
    return this.fakeStatus;
  }
}
...
    stubMatcher.onCall(i).callsFake(async () => {
      if (status < 200) {
        return new BadResponse(body, status);
      }
      if (typeof body === 'string') {
        return new Response(body, { status });
      }
      return new JSONResponse(body, status);
    });
...

That along with removing use of File (which in the end we should be doing anyway given that the actual fileFromSync method being mocked is now returning a Blob instance) may let us remove node-fetch also from the devDependencies.

[rpl]

This part of the changes in this PR makes me wonder if changing it to Blob would be hitting an issue when used against the real AMO server API, despites the tests in this repo as passing just fine.

In particular my doubt was related to the change in behavior I was expecting on the form-data being submitted:

• I was expecting the old version to have the side effect of including the filename derived from the file path in the form-data details being sent to the AMO API
• while the new version using Blob was going to likely be missing that bit (well, to be precise it would have the filename in the form-data details, but it would be set to "blob").

That is actually a bit that the AMO server API actually cares about, and if the filename associated to the form-data isn't ending with .zip, .xpi or .crx it actually fails with an explicit error message ("Unsupported file type, please upload a supported file (.crx, .xpi, .zip).").

That is unfortunately something that doesn't seem to be covered explicity by the web-ext tests.

I took a look to what node-fetch (well actually fetch-blob was doing to mock the File constructor, and it seems that in the short term we could mock it with something like the following (and then get rid of it once we bump the nodejs version required by web-ext to one that includes the File constructor natively):

...
import { basename } from 'path';
...
// Used by fileFromSync method to make sure the form-data entry will
// include a filename derived from the file path.
//
// TODO: Get rid of this hack when we will bump the web-ext nodejs
// version required to nodejs v20 (where the native File constructor
// exists).
export class FileBlob extends Blob {
  #name = '';

  constructor(fileBits, fileName, options) {
    super(fileBits, options);
    this.#name = String(fileName);
  }

  get name() {
    return this.#name;
  }

  get [Symbol.toStringTag]() {
    return 'File';
  }
}

export default class Client {
  ...
  fileFromSync(filePath) {
    // create a File blob from a file path, and ensure it to have the file path basename
    // as the associated filename, the AMO server API will be checking it on the form-data
    // submitted and fail with the error message:
    // "Unsupported file type, please upload a supported file (.crx, .xpi, .zip)."
    const fileData = readFileSync(filePath);
    return new FileBlob([fileData], basename(filePath));
  }
...
...

I tested this version on the actual AMO API and this seems enough to make it happy (and willing to accept the upload zip file to be signed :-))

It would be nice to also cover this explicitly, the following unit test seems one that may be quick enough to add and it seems to pass with both the FileBlob hack as well as the native File constructor available in nodejs 20:

    describe('fileFromSync', () => {
      it('should return a File with name set to the file path basename', () =>
        withTempDir(async (tmpDir) => {
          const client = new Client(clientDefaults);
          const FILE_BASENAME = 'testfile.txt';
          const FILE_CONTENT = 'somecontent';
          const filePath = path.join(tmpDir.path(), FILE_BASENAME);
          await fsPromises.writeFile(filePath, FILE_CONTENT);
          const fileRes = client.fileFromSync(filePath);
          assert.equal(fileRes.name, FILE_BASENAME);
          assert.equal(await fileRes.text(), FILE_CONTENT);
          assert.equal(String(fileRes), '[object File]');
        })
      );
    });

[fregante]

That worked on first paste! 🙌 Which is great because I was ready to throw in the towel 😅

[rpl]

👋 bye bye node-fetch, yours @rpl and @fregante 🤣

❤️ lovely to see this one also ready to go!!!

I'm signing it off and merging it