Make hanami-router compatible with Rack 2.0 only

Question

Make hanami-router compatible with Rack 2.0 only

jodosha opened this issue 8 years ago · 8 comments

Answer 1 · 2017-01-05T13:31:09.000Z

I found the problem to be this part:

request.env['SCRIPT_NAME'].must_equal script_name
request.env['SCRIPT_NAME'].must_be_kind_of(String)

request.env['PATH_INFO'].must_equal ''
request.env['PATH_INFO'].must_be_kind_of(String)

(https://github.com/hanami/router/blob/master/test/request_url_test.rb#L43)

With Rack 2.0.1 SCRIPT_NAME and PATH_INFO are swapped. Any hints how/why this happens?

Answer 2 · 2017-01-05T13:42:17.000Z

@pascalbetz Please investigate, I don't know why.

Answer 3 · 2017-01-06T07:40:03.000Z

Infos from gitter chat (thx @jodosha):

@pascalbetz @AlfonsoUceda This is the change that you're looking for https://github.com/rack/rack/blob/4b33af1c80c822cbcbb69113ff1e54f9454921c1/lib/rack/request.rb#L94 IIRC
Until Rack 1.6, when you did request.dup, the env of that request, was NOT duped.
They fixed it with that line above

Answer 4 · 2017-01-06T09:19:12.000Z

After digging through http_router and rack and hanami-router

The Rack Request is duplicated in http_router in a dynamically generated method in Node::Path#to_code (https://github.com/joshbuddy/http_router/blob/master/lib/http_router/node/path.rb#L32)

Answer 5 · 2017-01-06T23:23:59.000Z

There is a an open PR at http_router (joshbuddy/http_router#48) which would fix the problem.

Wait until it gets merged
Monkey Patch http_router
Stick with rack 1.6
Replace http_router

Given that 0. is unlikely to happen and there are plans to replace http_router sometimes I'd go for 1. Did I miss something?

Answer 6 · 2017-01-06T23:41:03.000Z

Thanks for the detailed analysis. Let me reply line by line. 0. http_router isn't maintained anymore. We can't wait for a merge. 1. This is the viable solution: change the http_router version in our gemspec to a strict release number and monkey-patch. 2. As we're approaching 1.0, we should get rid of the old deps that we don't want to maintain for the upcoming years. 3. I tried to hack for a couple of hours and use roda instead. It's a task that would take a couple of months. This would delay our 1.0. Also, we have battle tested http_router integration, a major change before 1.0, would require even more time to make this library mature again. I'm for solution 1. http://lucaguidi.com

…

On 07 gen 2017, at 00:24, Pascal Betz ***@***.***> wrote: There is a an open PR at http_router (joshbuddy/http_router#48) which would fix the problem. Wait until it gets merged Monkey Patch http_router Stick with rack 1.6 Replace http_router Given that 0. is unlikely to happen and there are plans to replace http_router sometimes I'd go for 1. Did I miss something? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

Answer 7 · 2017-01-06T23:48:24.000Z

@jodosha thx for feedback. Now that I think about there is also

clone and release our own version of http_router

Answer 8 · 2017-01-07T09:08:54.000Z

4. Do we want to support it for the upcoming years? My answer is no 😉 http://lucaguidi.com

…

On 07 gen 2017, at 00:48, Pascal Betz ***@***.***> wrote: @jodosha thx for feedback. Now that I think about there is also clone and release our own version of http_router — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.