Skip to content
This repository has been archived by the owner on Apr 5, 2024. It is now read-only.

Add hapi-require-https. #184

Closed
wants to merge 2 commits into from
Closed

Add hapi-require-https. #184

wants to merge 2 commits into from

Conversation

XhmikosR
Copy link
Collaborator

@XhmikosR XhmikosR commented Mar 8, 2018

If the production script is running npm start this will work out of the box. Otherwise we might need to set the env var NODE_ENV to production.

Requires the previous PRs merged.

Fixes #122.

/CC @fmarier

@@ -151,6 +152,15 @@ server.register(inert, () => {
});


if (process.env.NODE_ENV === 'production') {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could check here for another environment variable if present. Not sure if Heroku is used and what is set exactly, so let me know.

@XhmikosR
Copy link
Collaborator Author

XhmikosR commented Apr 6, 2018

ping @fmarier

@XhmikosR
Copy link
Collaborator Author

XhmikosR commented Jun 4, 2018

Ping @fmarier

@XhmikosR
Copy link
Collaborator Author

XhmikosR commented Jun 4, 2018

On a side note, I have personally switched to Node.js 10.x. Should I make the same here too? Otherwise I can't rebase/update the deps or anything.

@MatthewHerbst
Copy link

@XhmikosR since this is open-source, it might be good to wait until October when Node.js 10 gets LTS.

@XhmikosR
Copy link
Collaborator Author

XhmikosR commented Jun 4, 2018

I don't see how it's related. I use it, and I'm not going to install 8.x to make changes to this project only.

@MatthewHerbst
Copy link

MatthewHerbst commented Jun 4, 2018

For easy Node switching you can use nvm or nodenv. You can set them up (simple script) to listen to the value of any .node-version file or the engines listing within package.json.

It's related because some people may not be able to use Node 10 in their work environments due to it not being LTS yet. You don't know how else this code is being used or by whom, and imho, it seems unnecessarily restrictive to force users to use Node 10 for this code.

@XhmikosR
Copy link
Collaborator Author

XhmikosR commented Jun 4, 2018

I know all of these, but I'm on Windows. Sorry, I'm not going to change my workflow. The node 10.x switch seems fine to me, I'm using it for months without an issue.

I don't see anyone else doing anything on this project apart from me.

@XhmikosR
Copy link
Collaborator Author

XhmikosR commented Mar 4, 2019

@mozfreddyb: can you share a few details about the Heroku env? Here I rely on an environment variable which I assume is set on Heroku to force the redirect to https.

@mozfreddyb
Copy link
Contributor

I don't think we're setting any env variables in heroku itself. It just does npm start

@XhmikosR
Copy link
Collaborator Author

XhmikosR commented Mar 4, 2019

OK, then this should work out of the box since I'm setting NODE_ENV to production in the npm start script.

Can we test this somehow before merging it?

@mozfreddyb
Copy link
Contributor

I don't think so. I'll carve out some time later this week to test-deploy and roll back if things go horribly wrong

@XhmikosR
Copy link
Collaborator Author

XhmikosR commented Apr 8, 2019

@mozfreddyb: how do you want to proceed with this? https://srihash.org/ doesn't even respond anymore.

@XhmikosR
Copy link
Collaborator Author

I'm gonna close this for now. I will keep the branch in this repo if we need it again.

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

Successfully merging this pull request may close these issues.

www.srihash.org doesn't redirect to HTTPS when srihash.org (no www) does
3 participants