-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Added HTTPS option for dev server #735
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can add this, asking other to review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @doctyper! The code looks great.
I am wondering what the use case for this feature is? Normally I would recommend using build-storybook
and then serving the artifacts with static file serving. If there is a use case for using the development server with https
, then I'd be up for merging this since it doesn't add too much complexity to the code.
src/server/index.js
Outdated
} | ||
|
||
const sslOptions = { | ||
ca: fs.readFileSync(program.sslCa, 'utf-8'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@doctyper looking at the docs, it seems like ca
should be an array, and also is only used for self-signed certificates?
https://nodejs.org/api/https.html
https://nodejs.org/api/tls.html#tls_tls_createserver_options_secureconnectionlistener
If it's straightforward can you provide simple instructions for me to test this?
@arbesfeld Thanks. The use case is pretty straightforward. We have an HTTPS-everywhere policy @nfl. Some of our components change state depending on user login state, but this requires access to cookies. Since our cookies are secure, they can only be read from a domain with a valid SSL cert. Hence the need. I think I'll eventually stub that out so that I don't need actual cookies to display my component's state in Storybook, but this was a pain point during my prototype phase where I just wanted something quick and dirty to build my component. I'm sure similar use-cases exist for other SSL-sensitive APIs like Service Workers, LocalStorage, SessionStorage, etc. |
@shilman Oh interesting. It works fine as a string. I'll update the code to handle either case, and make |
Per the Node docs, the `ca` value should be an Array: https://nodejs.org/api/tls.html#tls_tls_createserver_options_secureconnectionlistener
… into feat/storybook-https
@shilman Done. Also, it's out of scope for this PR, but I was thinking about adding support for a |
src/server/index.js
Outdated
} | ||
|
||
const sslOptions = { | ||
ca: program.sslCa.map(ca => fs.readFileSync(ca, 'utf-8')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@doctyper I think you need to make this line conditional:
/Users/shilman/projects/react-storybook/dist/server/index.js:108
ca: _commander2.default.sslCa.map(function (ca) {
^
TypeError: Cannot read property 'map' of undefined
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repro:
MBP:react-storybook shilman$ ./dist/server/index.js --https --ssl-key ~/tmp/a.txt --ssl-cert ~/tmp/b.txt --port 6003
@doctyper Interesting idea about the config file. I agree that the growing list of command-line args is unwieldy, but I think the I agree that an alternative solution is outside the scope of this PR. |
@shilman I can add the new args to the list of env vars if you'd like. Not a problem. |
@doctyper I'm fine either way regarding the env vars, just wanted to point out the inconsistency if it wasn't intentional. Thanks for the quick fixes and the great feature!! |
@arbesfeld In addition to @doctyper's use case mentioned above, I think once we open up the server-side plugins as discussed in the video chat yesterday, there will be a lot of cases where one will want to run the server under HTTPS rather than serving from static files. (Whether this is best done by extending storybook-server or just pointing your client-side plugins at some completely different server is debatable.) |
I was thinking, maybe at some point in the future we could create middleware/routers for stuff like express. That way people would be in full control of the web-app and storybook would just be a route on that app. |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit ee78c90. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
This pull request introduces four CLI flags allowing users to serve Storybook over HTTPS: