[Miniflare] Request: add support Node's Response in cache `.put()`

[alexanderniebuhr]

Expected behavior:

Miniflare local Cache mocking should work with NodeJS's Response

Information:

Using the following code we get DevalueError: Cannot stringify arbitrary non-POJOs

import { Miniflare } from 'miniflare';
const _mf = new Miniflare({
  modules: true,
  script: '',
  cache: true,
  cachePersist: true,
  cacheWarnUsage: true,
  d1Databases: D1Bindings,
  d1Persist: true,
  r2Buckets: R2Bindings,
  r2Persist: true,
  kvNamespaces: KVBindings,
  kvPersist: true,
});
await _mf.ready;
const mfCache = await _mf.getCaches();
await runtime.caches.default.put(
  'http://localhost/cache',
  new Response('true', {
    headers: { 'Cache-Control': 'max-age=3600', 'X-Key': 'value' },
  })
);

However using the Response from Miniflare, we are able to put to cache:

import { Miniflare, Response as MiniflareResponse } from 'miniflare';
const _mf = new Miniflare({
  modules: true,
  script: '',
  cache: true,
  cachePersist: true,
  cacheWarnUsage: true,
  d1Databases: D1Bindings,
  d1Persist: true,
  r2Buckets: R2Bindings,
  r2Persist: true,
  kvNamespaces: KVBindings,
  kvPersist: true,
});
await _mf.ready;
const mfCache = await _mf.getCaches();
await runtime.caches.default.put(
  'http://localhost/cache',
  new MiniflareResponse('true', {
    headers: { 'Cache-Control': 'max-age=3600', 'X-Key': 'value' },
  })
);

Looking at Miniflare code, it seems like related to the reducers: https://github.com/cloudflare/miniflare/blob/79033b20d82fa672c9971f8c442321767b80e3c1/packages/miniflare/src/plugins/core/proxy/client.ts#L53-L59

Going further to confirm, it actually looks like indeed it would not work with NodeJS Response: https://github.com/cloudflare/miniflare/blob/79033b20d82fa672c9971f8c442321767b80e3c1/packages/miniflare/src/plugins/core/proxy/client.ts#L53-L59

However I also found the following section, which suggest it should work with NodeJS Response: https://github.com/cloudflare/miniflare/blob/79033b20d82fa672c9971f8c442321767b80e3c1/packages/miniflare/src/plugins/core/proxy/types.ts#L18-L36

[alexanderniebuhr]

As the current behavior is expected (see Discord), I still would like to request support for Node's Response

[alexanderniebuhr]

For info: https://discord.com/channels/595317990191398933/891052295410835476/1167830896603299910 Somtimes it looks like the instanceof check does not work for us in Astro: https://github.com/cloudflare/miniflare/blob/90d04ec119bf1aef0f70443c4dfd7a952ecc6322/packages/miniflare/src/workers/core/devalue.ts#L145-L147

[alexanderniebuhr]

Patching it like this, resolves our issue. However I don't think that is the final solution:

https://github.com/withastro/adapters/blob/811bb80e460b6b7f3846d5040121f6958c356225/patches/miniflare%403.20231010.0.patch

[bhvngt]

@alexanderniebuhr Thanks for the patch. I am facing similar issue when trying to store Node Response object into miniflare cache. I tried your patch. However, it does not appear to be working.

To give you a context, currently I have createResponse function that conditionally creates miniflare response if it is dev environment as follows.

async function createResponse(bodyInit?: BodyInit|MfBodyInit, options?: ResponseInit) {
  if (dev) {
    const {Response: MfResponse} = await import("miniflare");
    return new MfResponse(bodyInit as MfBodyInit, options as MfResponseInit) as unknown as Response;
  }
  return new Response(bodyInit as BodyInit, options);
}

I would have ideally liked to store Node Response object and not have this conditional logic. After applying your patch and then returning node response object (commenting out the conditional logic), I still end up getting the DevalueError: Cannot stringify arbitrary non-POJOs error.

Any pointers on if I am doing anything wrong will be very much appreciated.

[alexanderniebuhr]

I would add some debug logging in the Miniflare code, to see if the reducers work, so if you log val, you'll see logs multiple times. Some are Response (hopefully), and for that condition for Response should return true, just play around with the if condition until you get it working.

If that is not the issue (and the reducers work fine, but you still get non-POJOs), you would need to go deeper into the tree (look at the stack trace to see all the files/functions to look at)

https://github.com/cloudflare/miniflare/blob/90d04ec119bf1aef0f70443c4dfd7a952ecc6322/packages/miniflare/src/workers/core/devalue.ts#L132-L150

[mrbbot]

Hey! 👋 Thanks for raising this feature request. I'm going to transfer this to workers-sdk as that's the new home for Miniflare 3, and that will make it easier for us to track this. I think even if we decide not to add support for Node's own classes here, we should definitely make the error message clearer. 👍

[alexanderniebuhr]

I think even if we decide not to add support for Node's own classes here, we should definitely make the error message clearer.

@mrbbot Thanks for answering. Making the error message clearer makes total sense to me.

If you decide to not add support, I would appreciate if you could give at least some guidance how to handle a case where vite dev is used locally with a seperate miniflare/workerd process sidecar for bindings & cache and a framework, e.g. Astro. Because code which would run without an issue on Cloudflare new Response(), would break locally, because new Response() in vite dev, would be Node's version. And importing the Response from miniflare/workerd and use that, would break support for production.

[bhvngt]

Currently, I have created following function as a workaround to ensure that MiniflareResponse is used for dev setup. It is less than ideal but works for my setup.

  private async createResponse(bodyInit?: BodyInit, options?: ResponseInit) {
    if (dev) {
      const { Response: MfResponse } = await import("miniflare");
      return new MfResponse(bodyInit as MfBodyInit, options) as unknown as Response;
    }
    return new Response(bodyInit as BodyInit, options);
  }

[mrbbot]

@alexanderniebuhr not ideal, but could you try overriding globalThis's Request, Response, File, FormData, Headers and fetch with versions imported from miniflare in a "setup" script? Using Miniflare's versions of these classes in your user code has other advantages, like support for using fetch() as a WebSocket client, and access to the Request#cf property.

[alexanderniebuhr]

Yeah that would work for user generated code. But Astro internal also relies on globalThis.Response, so I'm not sure if we could actually override it. Our best bet would be probably to suggest users to use @bhvngt workaround, however that is not the best DX.

So if there is anything Miniflare/workerd could do to improve this, it would be great!