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

Support clients on Cloudflare workers #577

Open
juanpmarin opened this issue Apr 10, 2023 · 15 comments
Open

Support clients on Cloudflare workers #577

juanpmarin opened this issue Apr 10, 2023 · 15 comments
Labels
enhancement New feature or request

Comments

@juanpmarin
Copy link

Describe the bug

Hi! Thank you for the development of connect
I'm using connect-web to do some service-to-service communication, the client service is deployed in Cloudflare workers and it appears that Cloudflare workers do not support the mode field that connect is using in the request initializer
I'm getting the error:

ConnectError: [internal] The 'mode' field on 'RequestInitializerDict' is not implemented.

They're probably never implementing that field because it does not make sense in the context of Workers

It would be awesome if you expose some option to unset that field from the request initialization

https://github.com/bufbuild/connect-es/blob/6d97256107d128e48f4e298e0be19522ccc5b658/packages/connect-web/src/connect-transport.ts#L146

To Reproduce

Try to call some connect service from cloudflare workers using connect-web

Environment (please complete the following information):

  • @bufbuild/connect-web version: 0.8.5
  • framework and version: Cloudflare Workers (is not frontend but it uses browser apis in the backend)
@juanpmarin juanpmarin added the bug Something isn't working label Apr 10, 2023
@juanpmarin
Copy link
Author

juanpmarin commented Apr 10, 2023

@juanpmarin
Copy link
Author

@timostamm
Copy link
Member

Hey Juan, I've seen this issue before, it's really unfortunate that the Cloudflare implementation raises an error. As a library, it's not really feasible to detect the runtime. But I can also see where they are coming from, and that they want to be very clear about what features are supported.

We could avoid the issue for credentials, because "same-origin" is the default. We can simply not set the property here.

For mode, it's not as straight-forward, unfortunately. Current chrome uses "cors" as default, but the spec mentions "no-cors" as the default. We will also add support for GET requests soon, and we really don't want responses to be opaque.

So I'm afraid we don't have an immediate fix for this. We did just merge a feature that will solve this issue, though: #575 adds fetch API clients and handlers. The following Transport for the Connect protocol uses fetch(), but does not set credentials or mode:

import {createFetchClient} from "@bufbuild/connect/protocol";
import {createTransport} from "@bufbuild/connect/protocol-connect";

const transport = createTransport({
  httpClient: createFetchClient(fetch),
  baseUrl: "https://demo.connect.build",
  // ...
});

We still have to make this more convenient to use, but the next release will give you this option.

@juanpmarin
Copy link
Author

Hi @timostamm, I'll try the new transport as soon it is available, thanks!

@timostamm
Copy link
Member

It was just released in v0.8.6! Any feedback is much appreciated, Juan 🙂

@juanpmarin
Copy link
Author

juanpmarin commented Apr 11, 2023

@timostamm can you build an example of how to implement it, please?
I've tried the one that you sent but it needs a lot of parameters like: useBinaryFormat, interceptors, acceptCompression, sendCompression, compressMinBytes, readMaxBytes, writeMaxBytes

And I don't know what values to use there

@timostamm
Copy link
Member

Yes, that's part of what I was referring to with "more convenient to use" 🙂

You can use the following options:

        useBinaryFormat: false,
        acceptCompression: [],
        compressMinBytes: 0,
        interceptors: [],
        readMaxBytes: 1024 * 1024, // 1 MiB
        writeMaxBytes: 1024 * 1024, // 1 MiB
        sendCompression: null,

@juanpmarin
Copy link
Author

Awesome! it worked. Is there a place to follow the development of the more convenient API?

@timostamm timostamm changed the title ConnectError: [internal] The 'mode' field on 'RequestInitializerDict' is not implemented. Connect transport from @bufbuild/connect-web raises an internal error when running on Cloudflare workers Apr 13, 2023
@timostamm
Copy link
Member

We don't have a good place for Cloudflare support yet. #550 is specific to handlers. Let's use this issue for the time being 🙂

@juanpmarin
Copy link
Author

juanpmarin commented Apr 14, 2023

@timostamm It worked well on cloudflare workers but, when running on nodejs, it fails with the following error:

11:47:40 AM [vite] Internal server error: RequestInit: duplex option is required when sending a body.
  File: /home/juan/Projects/every/node_modules/.pnpm/@[email protected]_@[email protected]/node_modules/@bufbuild/connect/dist/esm/protocol/universal-fetch.js:36:12
  34 |  }
  35 |  function universalClientRequestToFetch(req) {
  36 |      return new Request(req.url, {
     |              ^
  37 |          method: req.method,
  38 |          headers: req.header,

I use nodejs for local development and then deploy to cloudflare workers

@timostamm timostamm changed the title Connect transport from @bufbuild/connect-web raises an internal error when running on Cloudflare workers Support clients on Cloudflare workers Apr 27, 2023
@timostamm timostamm added enhancement New feature or request and removed bug Something isn't working labels Apr 27, 2023
@juanpmarin
Copy link
Author

juanpmarin commented Jul 8, 2023

@timostamm Cloudflare workers now have support for TCP sockets, it would be awesome to have native support from connect

https://developers.cloudflare.com/workers/runtime-apis/tcp-sockets/

@mattolson
Copy link

@timostamm Just chiming in here to say that I'm also using Connect client on Cloudflare and this workaround is working for me. Don't know if I'm missing out on anything in the default connect client, but so far smoke test is fine.

@Arttii
Copy link

Arttii commented Mar 18, 2024

@juanpmarin did you find any workaround for this to work locally with node?

@juanpmarin
Copy link
Author

@Arttii yes, I use something like this:

import { createConnectTransport } from "@connectrpc/connect-web";
import { createFetchClient } from "@connectrpc/connect/protocol";
import { createTransport } from "@connectrpc/connect/protocol-connect";

export function createConnectFetchTransport(baseUrl: string) {
  const local = true; //here you should implement a way of knowing if you're running locally
  if (isLocal) {
    return createConnectTransport({
      baseUrl: baseUrl,
    });
  } else {
    return createTransport({
      httpClient: createFetchClient(fetch),
      baseUrl: baseUrl,
      useBinaryFormat: false,
      acceptCompression: [],
      compressMinBytes: 0,
      interceptors: [],
      readMaxBytes: 1024 * 1024,
      writeMaxBytes: 1024 * 1024,
      sendCompression: null,
    });
  }
}

There is a more appropriate way, that is implementing your own transport using fetch similar to what connect-web does but in a compatible way, but I haven't implemented it yet

@winterec
Copy link

winterec commented Jul 5, 2024

What's the current best practice for running @connectrpc/connect-web on Cloudflare Workers without nodejs-compat?

Are there any drawbacks of what I'm doing currently?

function fetchProxy(input: RequestInfo | URL, init?: RequestInit): Promise<Response> {
  // Create a new init object without mode and credentials
  const newInit: RequestInit = {...init};
  delete newInit.mode;
  delete newInit.credentials;
  newInit.redirect = 'manual';

  // Call the original fetch with the modified init
  return fetch(input, newInit);
}

const transport = createConnectTransport({
  baseUrl: 'http://localhost',
  fetch: fetchProxy,
});

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

No branches or pull requests

5 participants