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.

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

Add redirect helper #39

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

kevinresol
Copy link
Member

No description provided.

@back2dos
Copy link
Member

back2dos commented Jul 1, 2016

There are 3 issues I see with this:

  1. I'm not sure this is the right place for this to go. It feels more like this belongs on framework level, but maybe I'm wrong. Last I checked monsoon exposed such functionality also. I think it wouldn't hurt to try having a consensus here.
  2. Response body should be an argument and default to something human readable (in case you're using a client that doesn't follow redirects (e.g. cURL without -L)).
  3. What of the other redirection status codes? In particular 303 is important, since it is technically the correct one to use in PRG. If we want to pick, then I would say 302, 303 and 307 are the most useful ones, because of their different semantics. Permanent redirects are a great way of shooting yourself in the foot anyway. It wouldn't be a bad thing if people had to do that by hand :D

@kevinresol
Copy link
Member Author

kevinresol commented Jul 1, 2016

  1. I think redirect is as common as ofString or ofBytes. If they are included (and they are), probably redirect should be too.
  2. We can simply write 'Redirected: $url ($code $message)' in the body as default?
  3. What if changing the signature to redirect(url, ?code, ?message, ?body)?

@back2dos
Copy link
Member

back2dos commented Jul 3, 2016

I think redirect is as common as ofString or ofBytes. If they are included (and they are), probably redirect should be too.

I understand your argument, and FWIW I have already considered removing the implicit casts from String and Bytes. Maybe it's the right thing to do. But I would point out that the level of abstraction is a bit different. For String or Bytes we don't need to make any real choices. For a redirect, we do have to choose a default message body (which probably does not respect the language or content type requested by the client).

@kevinresol
Copy link
Member Author

Aright, never mind, I always keep in mind this comment:

Chances are that I will occasionally confront you with rather idiosyncratic decisions, that I just can't explain but "feel" to be right.

It is not super hard to put this redirect function in a helper class in my code base. So let's just don't waste time on such minor things. ;)

@kevinresol kevinresol closed this Jul 3, 2016
@back2dos
Copy link
Member

back2dos commented Jul 4, 2016

I'm not saying I'm against this either. I just want the library to have a really clear scope and we are in the process of shaping that.

My concern is to present HTTP as directly as possible. While working on this library, I realized how little I know about HTTP and discovered that much of my ignorance was rooted in working with APIs that try too hard to be convenient, sacrificing interesting or even important aspects.

For example, if you redirect to a 3rd party page that takes a long time to load or the user is on a really slow connection, they might sit in front of that page for 10-15 seconds or even longer. So it starts making sense to actually display a nice localized and styled page.

What I do wish to do is to encourage programmers (and framework authors in particular) to consider such aspects. Making the body mandatory may work toward that goal. Or not. I'm open to suggestions here.

I agree that redirection is just one detail. But in the end getting most of such details right is an important part in creating a well crafted web application, that always feels responsive to the user.

@back2dos back2dos reopened this Jul 4, 2016
@kevinresol
Copy link
Member Author

Ok, that sounds reasonable. But seems that at this moment the topic has gone beyond my knowledge, I am open to learn more but I am afraid that I don't have any deeper input right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants