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

Expose the raw event.request #52

Closed
dhrubabasu opened this issue May 11, 2021 · 14 comments
Closed

Expose the raw event.request #52

dhrubabasu opened this issue May 11, 2021 · 14 comments

Comments

@dhrubabasu
Copy link

https://github.com/lukeed/worktop/blob/master/src/request.ts#L4

$.request = request;

I intend on using this to do a passthrough handler:

export const logOnly: Handler = (request, _) => {
  log(await request.body.json())
  return await fetch(request.request);
};

As an aside, it also seems like you can't proxy websocket requests with worktop but you can with default workers. Exposing event.request would solve this as well.

https://community.cloudflare.com/t/websocket-pass-through-crashes-worker-script/78482/6

@lukeed
Copy link
Owner

lukeed commented May 11, 2021

function handler(req, res) {
  var request = new Request(req.url, req);
  // ...
}

@lukeed
Copy link
Owner

lukeed commented May 11, 2021

You can proxy WS just fine, there's even a worktop/ws submodule that handles this for you.

The entire framework works by passing a Response directly to the event.respondWith method. The only difference is that worktop adds extra avenues for you to creating/building up a Response

@dhrubabasu
Copy link
Author

import {Router, listen} from 'worktop';

const API = new Router();

API.add("POST", "/", async (request, response) => {
  // @ts-ignore
  const req = new Request("https://httpbin.org/post", request)

  return await fetch(req)
})

listen(API.run);

Raw response:

{
  "args": {},
  "data": "{\n\t\"kek\": \"kek\"\n}",
  "files": {},
  "form": {},
  "headers": {
    "Accept": "*/*",
    "Content-Length": "17",
    "Content-Type": "application/json",
    "Host": "httpbin.org",
    "User-Agent": "[CENSORED]",
    "X-Amzn-Trace-Id": "[CENSORED]"
  },
  "json": {
    "kek": "kek"
  },
  "origin": "[CENSORED]",
  "url": "https://httpbin.org/post"
}

With worktop:

{
  "args": {},
  "data": "function () { [native code] }",
  "files": {},
  "form": {},
  "headers": {
    "Accept": "*/*",
    "Content-Length": "29",
    "Content-Type": "application/json",
    "Host": "httpbin.org",
    "User-Agent": "[CENSORED]",
    "X-Amzn-Trace-Id": "[CENSORED]"
  },
  "json": null,
  "origin": "[CENSORED]",
  "url": "https://httpbin.org/post"
}

@lukeed
Copy link
Owner

lukeed commented May 11, 2021

Oh sorry, that's how it was in a previous version, before req.body became a function.

Instead, it's this:

const request = new Request(req.url, {
  ...req, body: await req.body.arrayBuffer(),
})

Playground

@lukeed
Copy link
Owner

lukeed commented May 12, 2021

@dhrubabasu Did this work for you?

@dhrubabasu
Copy link
Author

@lukeed It worked for the httpbin example but it doesn't appear to be working for websockets. Here's what I'm trying:

export const passThroughHandler: Handler = async (request, _) => {
  return fetch(
    // @ts-expect-error
    new Request(request.url, {
      ...request,
      body: await request.body.arrayBuffer(),
    }),
  );
};

API.add('GET', '/ws', passThroughHandler);

The result with the above worker:

% wscat -c "wss://example.com/ws"
error: Unexpected server response: 500

@lukeed
Copy link
Owner

lukeed commented May 12, 2021

What is your origin receiving?

@dhrubabasu
Copy link
Author

I haven't had time to dive into this further but wanted to post a workaround. I'm using worktop/cache so I modified it like this:

export function reply(handler: ResponseHandler): FetchHandler {
  return (event) => {
    if (['/ws'].includes(new URL(event.request.url).pathname)) {
      event.respondWith(fetch(event.request));
    } else {
      event.respondWith(
        lookup(event).then((previous) => {
          return (
            previous ??
            handler(event).then((response) => {
              return save(event, response);
            })
          );
        }),
      );
    }
  };
}

I'll try to put together a minimal repo this weekend to verify if there is an issue with worktop.

@lukeed
Copy link
Owner

lukeed commented May 13, 2021

Sure, thanks! You don't have to modify anything btw. Your initialization code can look something like this:

import { Router } from 'worktop';
import * as Cache from 'worktop/cache';

const API = new Router;

// ...

addEventListener('fetch', event => {
  let { pathname } = new URL(event.request.url);
  if (pathname === '/ws') event.respondWith(fetch(event.request));
  else Cache.reply(API.run)(event);
});

@lukeed
Copy link
Owner

lukeed commented May 13, 2021

My main hesitation here is that exposing a req.raw property makes it possible for you/users to access the same body multiple ways, which would throw an error. For example:

API.add('POST', '/demo', async (req, res) => {
  let foo = await req.body();
  // ^ success

  let bar = await req.body();
  // ^ this already throws
  //  but it's more obvious why

  let baz = await req.raw.text();
  // ^ throws, and not as obvious

  // also true for this sequence:
  await req.raw.text();
  await req.body(); // throws
});

The only valid way to access it twice would be to do this:

API.add('POST', '/demo', async (req, res) => {
  let copy = req.raw.clone();
  let input1 = await copy.text();

  let input2 = await req.body();
});

... which is fine, but is assumed knowledge & becomes the dev's responsibility.


The other mildly annoying thing is that req.cf and req.raw.cf would be identical. It's fine, but multiple ways to do the same thing kinda bug me 😅

Thoughts?

@dhrubabasu
Copy link
Author

dhrubabasu commented May 13, 2021

Honestly, I think the snippet you posted above is good enough. If you have to access the raw event.request, you're doing something that's not in the norm.

addEventListener('fetch', event => {
  let { pathname } = new URL(event.request.url);
  if (pathname === '/ws') event.respondWith(fetch(event.request));
  else Cache.reply(API.run)(event);
});

I think it should be documented though, perhaps under Advanced?

@ghost
Copy link

ghost commented Jun 28, 2021

I like the idea of passing the original request as request.raw so that you always have access to it.

For my use case I want to access the original request body reader, but that's currently not possible with Worktop without cloning the request.

This way you can take advantage of the worktop features but still fall back to original request handling where you need to.

The alternative is to build out 100% of functionality, which is maybe a good eventual goal, but until then request.raw may be a necessity.

@lukeed
Copy link
Owner

lukeed commented Sep 15, 2021

This is achieved through #83 and will be available as worktop@next for the time being.

@lukeed lukeed closed this as completed Sep 15, 2021
@janat08
Copy link

janat08 commented Nov 21, 2021

Couldn’t a proxy be used instead since then it’s no longer a function but a value ready for consumption which I think could take on form function if required for backwards compatibility.

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

No branches or pull requests

3 participants