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

feat(core, cli): builtin support of https and unix sockets #3831

Merged
merged 33 commits into from Sep 2, 2018
Merged

Conversation

galvez
Copy link

@galvez galvez commented Sep 1, 2018

TODO:

  • Review socket code
  • Adapt https PR into this
  • Write tests for sockets configurations
  • Write tests for https configurations

@galvez galvez added the WIP label Sep 1, 2018
@galvez galvez self-assigned this Sep 1, 2018
@galvez galvez requested review from clarkdo, pi0 and Atinux and removed request for clarkdo September 1, 2018 10:42
@galvez
Copy link
Author

galvez commented Sep 1, 2018

@pi0 @clarkdo followed your advice to the best of my judgement on https://github.com/nuxt/nuxt.js/pull/3812/files and https://github.com/nuxt/nuxt.js/pull/3403/files -- let me know if you have any additional feedback before I proceed to adding tests.

@codecov-io
Copy link

codecov-io commented Sep 1, 2018

Codecov Report

Merging #3831 into dev will decrease coverage by 0.05%.
The diff coverage is 96.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #3831      +/-   ##
==========================================
- Coverage   97.84%   97.79%   -0.06%     
==========================================
  Files          18       18              
  Lines        1162     1181      +19     
  Branches      317      324       +7     
==========================================
+ Hits         1137     1155      +18     
- Misses         24       25       +1     
  Partials        1        1
Impacted Files Coverage Δ
lib/common/nuxt.config.js 100% <ø> (ø) ⬆️
lib/core/nuxt.js 95% <96.42%> (-0.05%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f6a048...9423fcc. Read the comment docs.

lib/core/nuxt.js Outdated
const isHttps = !!this.options.http

if (isHttps) {
const httpsOptions = this.options.https === true ? {} : this.options.http
Copy link
Member

Choose a reason for hiding this comment

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

I suggest using options.server.* as well as setting defaults inside [nuxt.config.js](https://github.com/nuxt/nuxt.js/blob/dev/lib/common/nuxt.config.js#L18_

Copy link
Author

Choose a reason for hiding this comment

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

@pi0 good call, thanks!

lib/core/nuxt.js Outdated

let appServer
const isHttps = !!this.options.http

Copy link
Member

Choose a reason for hiding this comment

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

Use either http or https options!

Copy link
Author

Choose a reason for hiding this comment

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

@pi0 ops, that's a typo indeed!

@@ -77,8 +79,8 @@ if (nuxt.options.render.ssr === true) {
}
}

const { port, host } = nuxt.options.server
const { port, host, socket } = getLatestHost(argv)
Copy link
Member

@pi0 pi0 Sep 1, 2018

Choose a reason for hiding this comment

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

Shouldn't we add it to nuxt-dev too?

Copy link
Author

Choose a reason for hiding this comment

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

@pi0 good catch.

@galvez galvez removed the WIP label Sep 2, 2018
@pi0 pi0 changed the title feat: sockets and https feat(core, cli): builtin support of https and unix sockets Sep 2, 2018
@pi0 pi0 merged commit 7247968 into dev Sep 2, 2018
@pi0 pi0 deleted the sockets-and-https branch September 2, 2018 09:26
@lock
Copy link

lock bot commented Oct 31, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 31, 2018
@danielroe danielroe added the 2.x label Jan 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants