Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

[dev] Handle response headers #886

Merged

Conversation

EverlastingBugstopper
Copy link
Contributor

@EverlastingBugstopper EverlastingBugstopper commented Nov 20, 2019

Fixes #872

This is now a pretty usable dev server! There's still a bunch of stuff to do but this is what I'd considered an MVP.


worker code:

addEventListener('fetch', event => {
  event.respondWith(handleRequest(event.request))
})

async function handleRequest(request) {
  return new Response("hello world", { headers: {"x-test": "header"}})
}

Proxy and curl running in parallel

$ curl --head localhost:8000
HTTP/1.1 200 OK
content-length: 11
content-type: text/plain;charset=UTF-8
x-test: header
date: Wed, 20 Nov 2019 17:33:25 GMT
$ wrangler dev
Listening on http://localhost:8000
[2019-11-20 11:33:25] "HEAD example.com/ HTTP/1.1"

@EverlastingBugstopper EverlastingBugstopper self-assigned this Nov 20, 2019
@EverlastingBugstopper EverlastingBugstopper added this to the Preview Proxy milestone Nov 20, 2019
@EverlastingBugstopper EverlastingBugstopper changed the base branch from master to avery/look-ma-real-headers November 20, 2019 17:34
@EverlastingBugstopper EverlastingBugstopper changed the title [proxy] Handle response headers [dev] Handle response headers Nov 20, 2019
@EverlastingBugstopper EverlastingBugstopper force-pushed the avery/look-ma-real-headers branch 2 times, most recently from f1e4c2d to 6a5a226 Compare December 2, 2019 20:59
Copy link
Contributor

@ashleymichal ashleymichal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would like to see a couple of comments again, but i won't block on it.

@@ -46,12 +47,32 @@ pub async fn dev(
let server_config = server_config.clone();
async move {
Ok::<_, failure::Error>(service_fn(move |req| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this closure deserves to either be a well named function in its own right, or to have a comment above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think i can do that since hyper doesn't re-export the type MakeServiceFn from what I can tell. i've simplified the closure as much as i can by pulling some if it out into a separate function though

src/commands/dev/mod.rs Show resolved Hide resolved
Copy link
Contributor

@gabbifish gabbifish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lil nit, otherwise lgtm. I'd love for it to be addressed though--this is substantial enough that I give the previous PR with the same feedback a request change :)

src/commands/dev/mod.rs Show resolved Hide resolved
src/commands/dev/mod.rs Outdated Show resolved Hide resolved
@EverlastingBugstopper
Copy link
Contributor Author

@gabbifish @ashleymichal i think i've addressed concerns here :)

@EverlastingBugstopper EverlastingBugstopper merged commit 0e3b39d into avery/look-ma-real-headers Dec 12, 2019
@delete-merged-branch delete-merged-branch bot deleted the avery/response-headers branch December 12, 2019 22:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants