dunglas / vulcain

🔨 Fast and idiomatic client-driven REST APIs.
https://vulcain.rocks
GNU Affero General Public License v3.0
3.51k stars 106 forks source link

feat: send 103 Early Hints responses #95

Closed chalasr closed 11 months ago

chalasr commented 1 year ago

This makes Vulcain sends 103 Early Hints responses when server-push isn't supported (with preload links). The patch might be very naive but here we are, first contrib in Go. Please tell me if there is something else to be done.

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

andrerom commented 1 year ago

There is a todo comment about early hints on line 251, can it be removed now or more changes needed?

chalasr commented 1 year ago

Todo removed, thanks @andrerom. Also I just added a test, sadly it's failing: The 103 response is sent as expected, but it does not have the Link headers. I must be missing something as that header should be forwarded to the 103 response before it is sent. Any idea?

chalasr commented 1 year ago

Also, I think that Server Push should now be opt-in (through a config option). By default, we should only use 103 responses as they are the new standard. This can be done in another PR of course.

Work ongoing in https://github.com/dunglas/vulcain/compare/main...chalasr:vulcain:firstclass-103?expand=1 It'd be great to get this PR resolved/merged first as the WIP depends on it.

pautier commented 1 year ago

Also, I think that Server Push should now be opt-in (through a config option). By default, we should only use 103 responses as they are the new standard. This can be done in another PR of course.

Work ongoing in https://github.com/dunglas/vulcain/compare/main...chalasr:vulcain:firstclass-103?expand=1 It'd be great to get this PR resolved/merged first as the WIP depends on it.

Hi guys do you think we can move forward with the @chalasr PR, we need it to use correctly VULCAIN :(

chalasr commented 1 year ago

Friendly ping :)

dunglas commented 1 year ago

I pushed a commit simplifying bit the code. I tested locally with Caddy, and it works as expected, the test failure is because the legacy proxy doesn't seem to support 103 properly and is still used by the test suite.

dunglas commented 1 year ago

Now that Go 1.20 is released, it should be possible (I hope), to add proper support for 103 Early Hints to the legacy server that we use in our tests suite using a director function: https://pkg.go.dev/net/http/httputil#ReverseProxy.Director

We'll have to keep supporting Go 1.19 until https://github.com/caddyserver/caddy/issues/5288 is fixed.

Another option would be to add a test for this feature in the Caddy module now that https://github.com/caddyserver/caddy/issues/5187 is merged (we should be able to re-enable the tests for the Caddy module of Mercure by the way, but that's not related to this project 😅).

Even if we follow this path, I would like that we add an example using a director function, at least because it will be displayed in the docs and we want to keep good examples of how to use Vulcain as a library without Caddy.

WDYT?

chalasr commented 1 year ago

Works for me, I can be back on this PR in March. Sorry for the late reply

dunglas commented 1 year ago

Caddy 2.7 is out. We can now drop support for Go 1.19!

dunglas commented 11 months ago

Thanks @chalasr. I fixed a tricky issue and some flaky tests. It's entirely green now 🎉

chalasr commented 11 months ago

Thank you!