Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

CloudFlare workers Response.type workaround #159

Closed
jimmed opened this issue Jan 5, 2023 · 7 comments
Closed

CloudFlare workers Response.type workaround #159

jimmed opened this issue Jan 5, 2023 · 7 comments
Labels

Comments

@jimmed
Copy link

jimmed commented Jan 5, 2023

I've been using wretch for a few years now, and I absolutely adore it!

Recently I started using it in the CloudFlare Workers runtime. I noticed that when handling a non-ok response, wretch attempts to access the response's type property.

wretch/src/resolver.ts

Lines 52 to 54 in c1fc7a6

if (response.type === "opaque") {
throw err
}

Unfortunately, CF's Response implementation defines a getter for type, which throws an error, as they don't implement this field (given it's a server runtime, not a browser).

https://github.com/cloudflare/miniflare/blob/9d96aaa66aa48eb1107c86fe24cbfd444249533c/packages/core/src/standards/http.ts#L637-L639

To work around this, I wrote a small middleware:

wretch().middlewares([
  (next) => async (url, opts) => {
    const response = await next(url, opts);
    try {
      Reflect.get(response, "type", response);
    } catch (error) {
      Object.defineProperty(response, "type", {
        get: () => "default",
      });
    }
    return response;
  },
])

I'm not sure whether you would prefer to add this to wretch's documentation somewhere, or change wretch's error handling logic such that it catches these errors from CF.

elbywan added a commit that referenced this issue Jan 6, 2023
@elbywan
Copy link
Owner

elbywan commented Jan 6, 2023

Hey @jimmed 👋

I've been using wretch for a few years now, and I absolutely adore it!

Thanks for reporting the issue - and for the kind words! ❤️

I'm not sure whether you would prefer to add this to wretch's documentation somewhere, or change wretch's error handling logic such that it catches these errors from CF.

Since this is a very specific use case and it seems like the CF implementation differs from the standard (which is never supposed to throw) I would prefer not to change the code.

So I added a new Limitations section to the readme file to cover this kind of issues and took the liberty to post the middleware code you wrote as a workaround (with all due credits) 😉.

@elbywan elbywan closed this as completed Jan 6, 2023
@yanielblanco
Copy link

yanielblanco commented Feb 2, 2023

https://github.com/cloudflare/miniflare/blob/9d96aaa66aa48eb1107c86fe24cbfd444249533c/packages/core/src/standards/http.ts#L637-L639

Hi @jimmed, I'm testing wretch on CloudFlare and it throws the same error when using the middleware.
This is my implementation:

import fecth from "node-fetch";
import FormData from "form-data";
import { default as Url } from "url";

const contextMiddleware = (next) => (url, opts) => {
  console.log("----------------------------");
  console.log(url);
  console.log(JSON.stringify(opts));
  console.log("----------------------------");
  return next(url, opts);
};

const cloudflareMiddleware = (next) => async (url, opts) => {
  let response = await next(url, opts);
  try {
    Reflect.get(response, "type", response);
  } catch (error) {
    // Throw error here ******************************
    Object.defineProperty(response, "type", {
      get: () => "default",
    });
  }
  return response;
}

class RestClient {
  constructor(
    url,
    headers = { "Content-Type": "application/json" },
    debug = false,
    onCloudflare = true
  ) {
    let applyMiddlewares = [];
    if (debug) {
      applyMiddlewares.push(contextMiddleware);
    }
    if(onCloudflare) {
      applyMiddlewares.push(cloudflareMiddleware);
    }
    this.connector = wretch(url)
      .polyfills({
        fetch: fecth,
        FormData: FormData,
        URLSearchParams: Url.URLSearchParams,
      })
      .headers(headers)
      .middlewares(applyMiddlewares);
  }
}

export default RestClient;

@jimmed
Copy link
Author

jimmed commented Feb 2, 2023

@yanielblanco are you using the CloudFlare service worker format, or module worker format? I don't know if it makes any difference, but I'm running that middleware via the service worker format.

@yanielblanco
Copy link

@jimmed Hi, thanks for your fast answer. I'm using service worker format. My issue happened when try to define property. In this moment I think that invoke property type and throw unimplemented error of worker that you comment here #159 (comment). I am new working with workers 😅. Thanks for your help.

@jimmed
Copy link
Author

jimmed commented Feb 2, 2023

I'm not sure what to suggest -- the exact middleware code I included in the original post is currently running in production in CF workers, and works perfectly for me.

@yanielblanco
Copy link

@jimmed Thanks 😅

@yanielblanco
Copy link

yanielblanco commented Feb 2, 2023

@elbywan Is it possible to add validation for cloudflare before calling the type property? I know you don't like to modify the code because it's a very specific case but I want to keep using wretch because it's awesome. I created a fork and I added a try catch block around response.type.

try {
  if (response.type === "opaque") {
      throw err;
  }
} catch (e) {
  console.log("Running on worker cloudflare...");
}

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants