feat: decouple from cross-fetch

[dirkluijk]

All right, this PR includes:

• Moving away from cross-fetch by just patching the global fetch which should be present in all modern runtimes and browsers. That would close issue Allow Passing Custom fetch Values to Override cross-fetch Usage #22 and Replace or drop "cross-fetch" since it is outdated and obsolete #23.
• It will now force Node 18, which closes issue Node minimum version #21 and removes the need to patch the DOMException.
• I added a new feature that automatically registers enables in beforeAll, resets in beforeEach and disables in afterAll (not yet documented). this feature was dropped
• The source code is now written in TypeScript and transpiled to JavaScript into the dist directory, including the type definitions and source maps.
• The package is now published in ESM format. That could be considered a breaking change. I personally believe everyone should push to modern web standards and use ESM, but that is only my personal opinion. I could revert this, allthough Vitest should be able to handle ESM (actually, it embraces ESM). Never mind, it was already being published in ESM format before.

[dirkluijk]

@IanVS I changed the workflow a bit, so you will have to approve it before it runs :)

[IanVS]

Overall this seems like a huge improvement. Writing the code in TS is a big win, and relying on the global fetch removes the dependency on cross-fetch, which is great.

I took a first-pass at reviewing this, but did not yet test it out in my own project. I have a few minor requests here, some questions about changes to the tests, and one larger request to break out the auto-enabling changes to its own PR, so I can review and weigh the pros and cons of that separately from the decoupling from cross-fetch. This is already a big PR, breaking that out would help get this landed more quickly.

Thanks for all the effort here!

[dirkluijk]

Pushes a new commit, changes:

• merged the interface and the object into a single class definition, did some rework on the code that merges the Mock and the additional methods into the FetchMock
• removed the autoEnable features
• restored some aliases that I forgot to add back in (my first plan was to rewrite the whole thing, removing a lot of aliases, but then decided that wasn't a good idea for an existing library...)
• reverted some changes in the tests
• fixed inconsistencies between overloads and the actual original implementation, by making the latter leading and making the types a little bit more flexible
• made createFetchMock the default export again
• adjusted the NPM scripts
• adjusted some methods to match the original signatures

[dirkluijk]

@IanVS I assume the next version to be 0.4.0 here. Let me know if you have different ideas.

[dirkluijk]

This is the only breaking change I would like to keep (and maybe should document).

This is because the original typings had:

export interface MockParams {
  status?: number;
  statusText?: string;
  headers?: string[][] | { [key: string]: string }; // HeadersInit
  url?: string;
  /** Set >= 1 to have redirected return true. Only applicable to Node.js */
  counter?: number;
}

export interface MockResponseInit extends MockParams {
  body?: string;
  init?: MockParams;
}

I changed this to:

export interface MockParams {
  status?: number;
  statusText?: string;
  headers?: [string, string][] | Record<string, string>; // HeadersInit
  url?: string;
  /** Set >= 1 to have redirected return true. Only applicable to Node.js */
  counter?: number;
}

export interface MockResponse extends MockParams {
  body?: string;
}

since otherwise you have duplicated properties, making it very fuzzy imho.

[IanVS]

👍 I think I'm on board with this, but let me stew on it just a bit.

[IanVS]

Yep, I've convinced myself, the way it was before made no real sense. We should indeed document this, but the best place is probably in the release notes.

[IanVS]

Thanks for making the changes I asked for! This looks good to me, and my tests passed when I patched it into my existing project that uses it (though I don't use it very extensively, TBH).

I notice one final change here that I want to ask about. Previously it was up to the user to provide vi to the createFetchMock function. Now, you're importing it straight from vitest. This makes me slightly nervous because it means we are relying on both this package and the user's code pulling the same version of vitest. We have a peer dependency on vitest, so theoretically it should work fine, but I've struggled more than I like to recall with different versions of packages being used by different dependencies causing very subtle and hard-to-troubleshoot bugs.

Can you talk a little about what you see as the pros & cons of this new approach here vs the explicit dependency injection we currently have? And furthermore, are you maybe willing to move that change to a separate PR (can group together with the auto-enabling if you'd like), so that we can keep this particular PR focused on decoupling from cross-fetch?

Other than that, I think this is ready to go!

[IanVS]

What would you think about extending https://www.npmjs.com/package/@tsconfig/node18?

[dirkluijk]

I "extended" (copied) the version of https://github.com/total-typescript/tsconfig/blob/main/tsc/no-dom/library.json. It's very similar, and basically another opinionated preset based on Matt Pococks TSConfig cheat sheet.

Most important settings are (in my opinion):

• module / moduleResolution should be either node16 or nodenext when using tsc to compile into ESM. This is the case for both versions, although one sticks to node16 and the other chooses nodenext. I am not sure if there is any difference between the two at this point. I see most projects use nodenext, so I wouldn't be surprised if they're just aliases with only semantic differences.
• target should be compatible with Node 18 or higher, es2022 in our case. This is the case for both versions.
• for libraries its important to use "declaration": true so we emit type-definitions as well (this important now, since we now only have .ts files in our source), and also "sourceMap": true for the same reason.
• the other compiler flags are just additional (opinionated) tweaks on strictness and other optimizations.
• the lib can indeed be upgraded to es2023 for Node 18+

[IanVS]

👍 sounds good.

I see most projects use nodenext, so I wouldn't be surprised if they're just aliases with only semantic differences.

Yeah right now they're the same, the difference is that "NodeNext" will always be the latest, so if new features are introduced, they'll get picked up without changing anything in the config.

[IanVS]

Yep, I've convinced myself, the way it was before made no real sense. We should indeed document this, but the best place is probably in the release notes.

[dirkluijk]

I notice one final change here that I want to ask about. Previously it was up to the user to provide vi to the createFetchMock function. Now, you're importing it straight from vitest. This makes me slightly nervous because it means we are relying on both this package and the user's code pulling the same version of vitest. We have a peer dependency on vitest, so theoretically it should work fine, but I've struggled more than I like to recall with different versions of packages being used by different dependencies causing very subtle and hard-to-troubleshoot bugs.

Can you talk a little about what you see as the pros & cons of this new approach here vs the explicit dependency injection we currently have? And furthermore, are you maybe willing to move that change to a separate PR (can group together with the auto-enabling if you'd like), so that we can keep this particular PR focused on decoupling from cross-fetch?

The signature change was one of the many changes I initially made. At that point it was more of a simple proof-of-concept, written from scratch, using this library as inspiration. When I started adding more and more back in, I started to consider it as some kind of fork (but still, with many different design decisions). Only later I decided to contribute this as a pull request to this project, and with that approach we have all the reason to be careful. I see no reason to break this interface now. That’s why reverted the change. 😉

Is there a difference? Well, that depends.

• When this library would use Vitest only as dependency, and creates the mocks by importing from Vitest as its own dependency, you could in theory allow users to have their own version of Vitest, even if it's a conflicting version. In case of conflicts, NPM will put "our" version of vitest in the directory node_modules/vitest-fetch-mock/node_modules/vitest and the users version of vitest would be located in node_modules/vitest. That is perfectly fine, as long as it's not part of any public interface (that is, as return value or as argument). But when you break this rule, you could get weird errors since the argument or return value is possibly not assignable to the users type-definition. Remember, it's usually TypeScript complaining here, since at runtime it might possibly still work fine. I believe this is the situation you are referring to. It's basically caused by library publishers that made a mistake here.
• However, in our situation Vitest is part of our public interface, since we extend Mock from @vitest/spy in our return types. In that case you must always specify it as devDependency and peerDependency. This should be 100% safe, since for the user, there is really only one package of Vitest being installed (in node_modules/vitest). There are two caveats: first, when the user doesn't have skipLibCheck enabled, I believe there might be some cases where you get type-check errors in case of inconsistencies, but they are rare and the best practice is to always disable lib checks. Secondly, if the installed version has some breaking change somewhere internally, it could still lead to unintended behavior. But this should not happen, and will probably not happen with a library as popular as Vitest.

With the peer dependency approach, there is not really any value over providing the vi instance as argument compared to importing it ourselves. The result is exactly the same - both in runtime behavior and in compiletime type-checking. That's why I removed it initially, but reverting is better to not break the interface. Alternatively, we could also mark it as optional.

[dirkluijk]

Added another small fix that prevented types/test.ts from type-checking before. That led me to making isMocking public again, and adjusting the ErrorOrFunction type-definition a bit.

I can't find any remaining issues anymore. I'm quite confident about this PR.

[IanVS]

Awesome work, thanks, let's

[IanVS]

whoops, spoke too soon, looks like there's a minor linting issue.

[dirkluijk]

fixed!

[IanVS]

Thanks for contributing this as a PR even though I'm sure it was much more annoying to do than just making a fork or new project.

If you'd like to make more breaking change PRs, for things like auto-enable and relying on peer dependencies, I'd be happy to review/merge those as well before making a 0.4 release (or maybe we just go to 1.0.0 now).

I think I'm on board with the peer dependency, though there are some edge cases that could still bite us, like npm automatically installing a peer dependency when it doesn't find one, and yarn doing weird things in monorepos sometimes. But I think in our case it's probably pretty low risk.

[dirkluijk]

Thanks for contributing this as a PR even though I'm sure it was much more annoying to do than just making a fork or new project.

No worries, it actually reminds me to care about breaking changes, which is absolutely a good thing!

If you'd like to make more breaking change PRs, for things like auto-enable and relying on peer dependencies, I'd be happy to review/merge those as well before making a 0.4 release (or maybe we just go to 1.0.0 now).

My feeling right now is to release this as 0.4, then add new feature(s) in 0.5, and then keep 1.0 for when things are stable and maybe for introducing some breaking changes and/or mark some things as deprecated to clean things up (and remove them for real in 2.0).

If you agree, I'd say, let's release and publish the new version. 🎉

I think I'm on board with the peer dependency, though there are some edge cases that could still bite us, like npm automatically installing a peer dependency when it doesn't find one, and yarn doing weird things in monorepos sometimes. But I think in our case it's probably pretty low risk.

As far as I can see, nothing changed there. Vitest was already a peer dependency before 😉