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.

Sign up for GitHub

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

Jump to bottom

Websocket crate incompatible with version 2 #63

Open
mrene opened this issue Mar 20, 2021 · 8 comments
Open

Websocket crate incompatible with version 2 #63

mrene opened this issue Mar 20, 2021 · 8 comments

Comments

@mrene
Copy link
Contributor

mrene commented Mar 20, 2021

But hyper_tungstenite works perfectly out of the box, since it works directly with a hyper::Request<Body>, the main example even uses the same function signature as routerify handlers.

Would it be okay to change the README reference?
@gsserge
Copy link
Member

gsserge commented Mar 20, 2021

Oh yeah, we actually use hyper-tungstenite at work too. The beauty of playing nicely with hyper is that so much is available out of the box.

@rousan would you be fine with sunsetting routerify-websocket in favor of hyper-tungstenite?

On a related note, I think we should bring routerify-query into the main repo and make it available via RequestExt trait similar to req.param(). It's almost always needed, and providing it as middleware might be a bit heavyweight.

@seanpianka
Copy link
Member

@rousan would you be fine with sunsetting routerify-websocket in favor of hyper-tungstenite?

I'm in favor of using the existing hyper ecosystem 👍

...we should bring routerify-query into the main repo and make it available via RequestExt trait similar to req.param(). It's almost always needed, and providing it as middleware might be a bit heavyweight.

👍 For each project I've used Routerify for, parsing of the querystring from a request is always needed. I think it's a great idea to merge this into RequestExt.

@rousan
Copy link
Member

rousan commented Mar 20, 2021

Okay, I will look into routerify-websocket.

@rousan
Copy link
Member

rousan commented Mar 20, 2021

routerify-query could be added in routerify repo itself, I released it as separate repo to make core routerify lightweight as much as possible.

We can merge it to core routerify and use feature flag to enable it.

@gsserge gsserge mentioned this issue Mar 21, 2021
Open
@ErwanDL
Copy link
Contributor

ErwanDL commented Jul 23, 2021

Hello @rousan @gsserge @seanpianka, are these two topics (one being moving routerify-query in the routerify repo and making it available via a feature flag, the other being deprecating routerify-websocket in favor of hyper-tungstenite) still on the agenda ? I am willing to help with these and create PRs.

I had started working on porting routerify-websocket to version 2 here routerify/routerify-websocket#4, but seeing the discussion here about sunsetting the project I now understand why it did not raise much interest. I was thinking of updating the README/docs and maybe (it may not even be necessary, if hyper-tungstenite really is easy to work with) adding an example using hyper-tungstenite, so that users of routerify v2 are not mislead into using routerify-websocket anymore, what do you think ?

@gsserge
Copy link
Member

gsserge commented Jul 23, 2021

Hey @ErwanDL, thanks for helping out with this!

I personally do not have much time working on routerify-websocket, and given that hyper-tungstenite works just fine, my preference would be to stick with the latter. The project is active and the maintainer is very responsive to PRs.

Speaking of query string parsing, again, my personal preference would be to leverage the url crate directly and add a few methods to RequestExt and PartsExt. Have query string parsing being a middleware seems a bit heavy. This could also give us an opportunity to fix the rare case of multiple values for the same query key (this can of course be fixed in the middleware itself, I'm just mentioning it here not to forget).

@ErwanDL
Copy link
Contributor

ErwanDL commented Jul 23, 2021

Sounds good, I will update the docs to refer to hyper-tungstenite and create a PR then.

Oh okay, I had not understood that the idea was to remove completely the query middleware and just put its functionality in RequestExt/PartsExt, but that sounds like a good idea. I'll work on this as well, I guess I'll create a PR this weekend or next week.

@gsserge
Copy link
Member

gsserge commented Jul 23, 2021 •
edited

Oh okay, I had not understood that the idea was to remove completely the query middleware and just put its functionality in RequestExt/PartsExt, but that sounds like a good idea. I'll work on this as well, I guess I'll create a PR this weekend or next week.

Oh, this is just my idea! I do not think that there's a consensus around this issue yet. But it does make sense, I think, since the info is local to the request and adding those couple of methods basically wrapping the ones from url should not be hard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@gsserge @mrene @rousan @seanpianka @ErwanDL