connectrpc / connect-es

The TypeScript implementation of Connect: Protobuf RPC that works.
https://connectrpc.com/
Apache License 2.0
1.26k stars 68 forks source link

Support clients on Cloudflare workers #577

Open juanpmarin opened 1 year ago

juanpmarin commented 1 year ago

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):

juanpmarin commented 1 year ago

Here some probe that they won't implement it

https://github.com/cloudflare/workerd/blob/42742874d853082232a5aedd285fdd61ce7237b2/src/workerd/api/http.h#L519-L526

juanpmarin commented 1 year ago

Same bug applies for the field credentials that you also set by default https://github.com/bufbuild/connect-es/blob/6d97256107d128e48f4e298e0be19522ccc5b658/packages/connect-web/src/connect-transport.ts#L144

timostamm commented 1 year ago

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: https://github.com/bufbuild/connect-es/pull/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 commented 1 year ago

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

timostamm commented 1 year ago

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

juanpmarin commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

timostamm commented 1 year ago

We don't have a good place for Cloudflare support yet. https://github.com/bufbuild/connect-es/issues/550 is specific to handlers. Let's use this issue for the time being πŸ™‚

juanpmarin commented 1 year ago

@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/@bufbuild+connect@0.8.6_@bufbuild+protobuf@1.2.0/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

juanpmarin commented 1 year ago

@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 commented 6 months ago

@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 commented 3 months ago

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

juanpmarin commented 3 months ago

@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 commented 3 days ago

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,
});