Add headerRedirect option #1

ryanburnette · 2018-08-21T14:01:11Z

ryanburnette commented

2018-08-21 14:01:11 +00:00

No description provided.

ryanburnette changed title from ~~Test~~ to Add headerRedirect option

2018-08-21 14:01:26 +00:00

ryanburnette commented

2018-08-21 14:11:18 +00:00

I propose that there be a headerRedirect option in case someone wants to use header redirects.

I respect your assertion that meta redirect is the best choice. I just think we should have the option in greenlock-express.

It would work something like this:

app.use('/', require('redirect-https')({
  headerRedirect: true
}));

... or ...

app.use('/', require('redirect-https')({
  headerRedirect: {
    responseCode: 307
  }
}));

I propose that there be a `headerRedirect` option in case someone wants to use header redirects. I respect your assertion that meta redirect is the best choice. I just think we should have the option in greenlock-express. It would work something like this: ```js app.use('/', require('redirect-https')({ headerRedirect: true })); ``` ... or ... ```js app.use('/', require('redirect-https')({ headerRedirect: { responseCode: 307 } })); ```

ryanburnette commented

2018-08-21 14:12:48 +00:00

The readme could be updated to say more broadly that this repository is for HTTPS redirect, not just HTTPS redirect using meta.

The documentation should still explain why meta is better in the opinion of the author, citing the blog post, but will offer header as an option and provide a usage example to that end.

The readme could be updated to say more broadly that this repository is for HTTPS redirect, not just HTTPS redirect using meta. The documentation should still explain why meta is better in the opinion of the author, citing the blog post, but will offer header as an option and provide a usage example to that end.

coolaj86 commented

2018-08-23 04:17:06 +00:00

A nit! A nit! I have a nit!

res.redirect only works in express. It won't work in other frameworks like koa, hapi, rill, etc.

Also, for reasons that I don't remember you do need to have a certain minimum number of bytes in the body of a redirect (which express probably does automatically).

I think this is what we need:

res.statusCode = code || 302;
res.end("some escaped html, probably the same as returned regularly");
return;

Would you mind updating and testing?

A nit! A nit! I have a nit! `res.redirect` only works in express. It won't work in other frameworks like koa, hapi, rill, etc. Also, for reasons that I don't remember you do need to have a certain minimum number of bytes in the body of a redirect (which express probably does automatically). I think this is what we need: ```js res.statusCode = code || 302; res.end("some escaped html, probably the same as returned regularly"); return; ``` Would you mind updating and testing?

ryanburnette commented

2018-08-25 06:01:02 +00:00

I don't mind. I will.

ryanburnette commented

2018-09-07 19:31:46 +00:00

I think this is a working PR now. I tried and failed to write a test. You may have to write it for me.

coolaj86 closed this pull request

2018-10-02 22:59:42 +00:00

Sign in to join this conversation.

No reviewers

No Label

No Milestone

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: coolaj86/redirect-https.js#1