Re: [whatwg/fetch] Proposal: Easier request/response rewrites (#671)

> Maybe instead of making the constructors support two purposes, their usual one and the copying use case of the OP, it'd be better to add a dedicated static factory method.

I'm fine with that, but I note that the Request constructor already appears to be designed to support this use case, by passing the request as the first parameter. It just seems to be missing a couple pieces. So since that's already there, I feel like filling it out is cleaner than creating another thing.

> I don't like duplicating `headers`'s functionality on `Request` ...  seems like a bad idea when the goal is to just save typing a `.`.

Since headers are mutable on newly-constructed objects, I can live with telling people to modify them there.

But it's a lot more than just typing a `.`. The way you rewrite headers is imperative, while the way you rewrite anything else is declarative. Those are two very different programming styles. Mixing them creates a fair amount of cognitive overhead. It's not intuitive to developers -- especially inexperiented ones -- why headers should work differently from everything else.

It doesn't help that when people try to "fix" their code to be consistent, it leads to weird failure modes. For example, say you have:

    addEventListener("fetch", event => {
      let request = new Request(event.request, {redirect: "follow"});
      request.headers.set("X-Example", "foo");
      event.respondWIth(fetch(request));
    })

You decide you don't like the style mixing, so you try to rewrite it to be all-declarative:

    addEventListener("fetch", event => {
      let request = new Request(event.request, {
        redirect: "follow",
        headers: { "X-Example": "foo" }
      });
      event.respondWIth(fetch(request));
    })

But it turns out this completely wipes out all the existing headers and replaces them. Whoops! But it seems to work because most request headers are not essential for GETs. You just silently lose things like caching (missing `If-Modified-Since`, etc.).

Or maybe you try to go the other way -- all-imperative:

    addEventListener("fetch", event => {
      let request = new Request(event.request);
      request.redirect = "follow";
      request.headers.set("X-Example", "foo");
      event.respondWIth(fetch(request));
    })

This produces no error, but the redirect mode change is silently ignored. (Well, it throws an exception in strict mode, but few people use strict mode, unfortunately.)

This can be really hard for inexperienced developers to wrap their heads around. And with CF Workers, we get a lot of very inexperienced developers trying to write code. (I suspect our developers are on average much less experienced then Service Worker develpoers.)

So my hope is that we can develop one common syntax for rewrites, with minimal warts, and get everyone to use that.

## Alternative Proposal

Maybe could introduce a new `rewrite()` method:

    let request = event.request.rewrite({
      url: "https://example.com/foo",
      redirect: "follow",
      headers: { "X-Example": "foo" }
    });

This mostly has the obivous semantics, except when it comes to the `headers` sub-object. Whatever you provide for `headers` gets forwarded on to `Headers.rewrite()`. So the new headers above would be computed as `event.request.headers.rewrite({"X-Example": "foo"})`. We would then define `Headers.rewrite()` as a merge, not a replacement. It's equivalent to making a copy of the Headers object then calling `set()` for each key/value pair -- or if the value is null, it could be treated like calling `remove()`.

I think this keeps `Request` and `Headers` loosely-coupled, since the semantics of header rewrites are defined by the `Headers` class. It has the down side that there's no obvious way to specify `append()` instead of `set()`, but that's rarely actually needed so maybe it doesn't matter.

Thoughts?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/fetch/issues/671#issuecomment-366350085