Skip to content
New issue

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

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

Already on GitHub? Sign in 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...");
}

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

No branches or pull requests

3 participants