feature(hyper): <insert *fireworks* here> #157
Conversation
Code of the form below will break, with no simple workaround:
fn handler(..) {
..
(status_code, str, vec![header, header, header])
}
[breaking-change]
This will break any code trying to perform undefined behaviour as mentioned in nickel-org#99. Anything that performed multiple sends, or modified headers after sending data will be affected by this. (Although that pretty much breaks HTTP anyway) [breaking-change] Closes nickel-org#99.
This will be necessary for an eventual move to unboxed closures. The majority of current Middleware that used the blanket impls should probably be valid if wrapped in a middleware! macro instead. Obviously a huge [breaking-change]
|
Cool! It wasn't as big of a change as I anticipated really, no huge architectural changes, so that's nice. I left a few comments but it looks nice overall. Super exciting news this! Thanks for the great work @Ryman! |
|
generally lgtm! having this merged sooner rather than later (even with some rough edges) would be nice so that catching up with Hyper is simpler and so that we can start using it. thanks @Ryman ! |
|
Lots of upstream change to new I tried to fix it up but each fix just leads to more compiler errors as it unveils more typechecking issues etc. Hyper's broken too but they have a patch already done that just needs to land, which then probably needs adjustment on this branch to accommodate but then we're good? |
|
yeah if we merge this, we can then all pitch in for polishing the rough edges and keeping up to date with hyper. |
|
This would be neat becuase currently, nickel fails to build due to rust-http |
|
@simonpersson Pushed the |
|
Looks nice to me! |
|
w00t! are we ready to rock/merge? :-) |
|
@simonpersson upstream's fixed, travis passing, merging! |
feature(hyper): migrate from rust-http to hyper
|
Yay! Great work @Ryman! |
|
awesome - thanks @Ryman ! |
This is basically feature parity with master, there's still a decent amount I'd like to change/introduce, but I feel this is in a state where it should be reasonably stable enough for review and can allow us to iterate together.
This obviously breaks a lot of code, and I'm especially unsure about the error handling. I've chosen to only allow error handlers to operate on a response that is already in progress (I still think
NickelErrorneeds a redesign but didn't want to sneak an opinionated rewrite into this). The reasoning is that either the handler knows what the error is (BadRequest, etc) and can give appropriate status and kick off the request or the error happens mid-stream and it would be a logic error to have an error handler try to edit theStatusCodemid-response. Thestreamis optional inNickelErroras there could be a write error, and then writing on the stream is useless, and the handlers point is just to log.Most blanket impls for
Middlewareare removed so that we only have one blanket impl (which will be a requirement for unboxed closures). I actually left in two since one is equivalent to an unboxed closure with ref captures which can be removed in future.I added convenience macros
middleware!to provide similar syntax as an unboxed closure as there are still tricky lifetime issues that need to be dealt with to get them working (I gave it a number of frustrating attempts before moving on with a more flexible macro based approach). I think it might actually be worth adding anerror_handler!variant and splitting the current macro intomiddleware!andhandler!, the former returningMiddlewareResult, the latterT: ResponseFinalizer.Documentation has only really been updated to pass tests, a lot of it could benefit from a rewrite, I don't feel it's a blocking issue.
Bare in mind this has been through a number of rebases, so please point out nits where I've botched something! Also open to naming bikeshedding for anything introduced here. I'll probably do some additional cleanup in the next day or two.
cc @simonpersson @cburgdorf @rgs1
This will fail travis until typemap & plugin merge upstream PRs.
Closes #99.