igor-alexandrov / wiselinks

If Turbolinks are not enough for you. Wiselinks makes your application work faster.
MIT License
723 stars 89 forks source link

Setting X-Wiselinks header field ignores trailing_slash #27

Closed jumph4x closed 11 years ago

jumph4x commented 11 years ago

Heyyo,

All requests appear as if not having a trailing slash via request.url (broken Rails convention, you could say). The correct and actual request URI is stored in request.env['REQUEST_URI']

When a request comes through with a trailing slash, Rails' request.url omits the trailing slash, creating a mismatch between the URL of wiselinks request and the X-Wiselinks header, causing the page:redirected event to be fired in all cases, effectively breaking a bunch of JS and so on and so forth.

jumph4x commented 11 years ago

Turbolinks already ran into this it seems: https://github.com/rails/turbolinks/pull/219

I ran into it trying to incorporate wiselinks on our eCommerce Spree based project.

jumph4x commented 11 years ago

Ignore first two referenced, had to rewrite history twice for a clean commit.

igor-alexandrov commented 11 years ago

For some reason sometimes request.env['REQUEST_URI'] contains full URL, but sometimes only path, so it causes redirects in wiselinks-0.7.0, because state.url always contains full url.

jumph4x commented 11 years ago

Same thing was happening with request.url, AFAIK (from reading source and documentation a while ago for rails 3.X) request.env[REQUEST_URI] is meant to contain the request as Rack hands it over to Rails.

Any additional insight? Rails/Rack version? Special (am assuming) production circumstances?

igor-alexandrov commented 11 years ago

We should check if request.env[REQUEST_URI].start_with?(request.protocol + request.host_with_port). If not, then add it.

jumph4x commented 11 years ago

Oh, I see what you meant now. I'll try do my best to prepare a patch this week.

How do you know this AKA how can I reproduce this? It almost sounds like a side effect of a specific stack or something. In other words, I highly doubt this inconsistency happens on the same setup between requests.

How did you find out about this?

igor-alexandrov commented 11 years ago

I can be reproduced when using Thin in development.

jumph4x commented 11 years ago

Interesting.

igor-alexandrov commented 11 years ago

Fixed this in wiselinks-0.7.1. Moved logic to CoffeeScript code.

jumph4x commented 11 years ago

Good to know :+1:

jumph4x commented 11 years ago

wiselinks 0.7.1 sends the incorrect (trailing slash lacking) header now, but doesn't seem to trigger a redirected event.

Is this intended?

igor-alexandrov commented 11 years ago

It was done as a hotfix, to make Wiselinks work. I normalize URL in Coffee, to prevent it from unnesessary redirects. The best approach would be to update Coffee to handle both full URL and path without a host as well. Then we could return to your solution with request.env[REQUEST_URL].

jumph4x commented 11 years ago

Gotcha.