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

Support HTTPS #274

Merged
merged 10 commits into from Oct 17, 2017
Merged

Support HTTPS #274

merged 10 commits into from Oct 17, 2017

Conversation

justanotherdot
Copy link
Contributor

This should hopefully resolve #178. It uses the example given in the
micro examples of with-https. It has some rough edges still that
needed to be sanded down:

  • Choosing a specific short flag name for ssl (and if the initial flag
    should still be called ssl; I'm tempted to rename it to https or what
    have you) - would like if someone chimed in about their design thoughts for this.

  • The example always acks with 200 and the example object - should just be a matter of passing off the handler as fn to microHttps.

  • It does not support compression and merely overwrites the previous
    server var - should also be trivial, just compress the handler before handing it off to whichever server will take it.

This should hopefully resolve #178. It uses the example given in the
`micro` examples of `with-https`. It has some rough edges still that
needed to be sanded down:

* Choosing a specific short flag name for ssl (and if the initial flag
should still be called ssl; I'm tempted to rename it to `https` or what
have you).

* The example always acks with 200 and the example object.

* It does not support compression and merely overwrites the previous
server var.
@justanotherdot
Copy link
Contributor Author

justanotherdot commented Oct 11, 2017

I should be able to get to last two bullet points without much delay; the first needs someone to chime in on design wise. I'm also aiming to do some general cleanup with what I've introduced so it's not entirely set in stone yet but I'm happy to oblige any changes needed. 🙂

This also calls `compress' on the handler when the `unzipped' flag is
not present.
@justanotherdot
Copy link
Contributor Author

Last commit should resolve the latter two bullet points; just need to know if we should change the flag name? (I'm going to vote 'yes'! Maybe https?)

@justanotherdot
Copy link
Contributor Author

justanotherdot commented Oct 11, 2017

Also just realized there should probably be:

  • a check for at least min of node v7.6 to support the async keyword unless it's unnecessary. Noticed that I had taken out the async keyword so this is no longer true.
  • a way to pass the cert + key manually if one is inclined to do so, although I think I can save that for another PR if people are inclined to have it. I'm not sure about the design goals of serve especially in the context of zeit now.

Looks like http-serve has --ssl and -S for the flags they use, so it might be ok to keep it and introduce -S here for consistency. and it looks like we're already using -S here, so not sure what to put as an alternative.

Copy link
Contributor

@leo leo left a comment

Choose a reason for hiding this comment

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

Fantastic PR! 😊

I added a few comments. I think we can release it after you've made the changes and then add support for custom certs if anyone is requesting it. I personally don't see a need for that.

Also please resolve the conflicts! 🙏

lib/options.js Outdated
},
{
name: 'ssl',
description: 'Serve with ssl',
Copy link
Contributor

Choose a reason for hiding this comment

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

Serve content using SSL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good; wasn't sure what the ideal copy was so I figured someone would eventually chime in 😉

key: cert.key,
cert: cert.cert,
passphrase: cert.passphrase
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a newline after this line 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 🙂

bin/serve.js Outdated
@@ -4,6 +4,7 @@
const path = require('path')

// Packages
const https = require('https')
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a package - it needs to be part of the "Native" import group.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing; force of habit to add to the end and glossed over the comments 🙂

@justanotherdot
Copy link
Contributor Author

Thanks! I'll be sure to run npm to update thepackage-lock.json 😄

@justanotherdot
Copy link
Contributor Author

justanotherdot commented Oct 16, 2017

Per lib/server.js conflicts, I think the global version of prettier I have installed may be reintroducing that (package.json shows v1.7.3 whereas I'm running v1.7.4). I'll resolve here to circumvent it running.

Per package-lock.json I'll just resolve to what master has for now.

@justanotherdot
Copy link
Contributor Author

justanotherdot commented Oct 16, 2017

Should be good, and should also be using the appropriate non-master (npm install) package-lock.json. 👍

I am a little worried I didn't update the ava snapshots correctly, though. It looks like the generated markdown swapped the position of the regular and minimist options. If someone can chime in about that I'd appreciate it 😄

@leo leo merged commit c6336ea into vercel:master Oct 17, 2017
@leo
Copy link
Contributor

leo commented Oct 17, 2017

Thank you so much! I'll make sure to update the snapshots and lockfiles to be really sure.

@justanotherdot justanotherdot deleted the justanotherdot/support-https branch October 17, 2017 10:27
@albinekb
Copy link

:nice: use of the micro example 👍

@justanotherdot
Copy link
Contributor Author

No worries, and thanks for writing that example, @albinekb! Make hacking up the diff a snap 👌

@albinekb
Copy link

albinekb commented Oct 17, 2017

OSS FTW! 💯 👊

@jadbox
Copy link

jadbox commented Aug 9, 2018

This needs documentation in the README.

@paullaffitte
Copy link

Does this feature has been disabled? The --ssl flag doesn't exists anymore, and neither nothing related to it in the --help flag.

@devinrhode2
Copy link

Indeed need documentation! I don't know what to do

@nermin99
Copy link

@justanotherdot What happened to your flag? The --ssl flag doesn't work anymore and there's only --ssl-cert and --ssl-key.

@justanotherdot
Copy link
Contributor Author

justanotherdot commented Aug 13, 2020 via email

@nermin99
Copy link

nermin99 commented Aug 13, 2020

@leo why was support for automatic HTTPS removed? As per #520 and #511 it is understandable that users might want to use manual certs and keys, but there is still, no even more, use cases for automatic SSL/HTTPS. One of the biggest reasons why serve is so popular is because it's a one-line command to literally "serve" your application. If a user wants or for some reason needs HTTPS they don't wanna have to go through a whole process of installing another tool just to get it.

Personally I stumbled upon serve when building my create-react-app and wanted to "serve" it. But because I use service workers and push notifications I need to serve it over HTTPS so I can test on other devices on the network. Sure I can get HTTPS for my self as per #520, but my co-workers can't, since they might not have their own manual certs and keys... you see the problem?

There is no reason why both manual and automatic HTTPS can't be supported.

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

Successfully merging this pull request may close these issues.

Option for enabling HTTPS
7 participants