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

New spdy option to enable/disable HTTP/2 with HTTPS #1721

Merged
merged 12 commits into from
Apr 4, 2019
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions bin/options.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,11 @@ const options = {
group: SSL_GROUP,
describe: 'HTTPS',
},
http2: {
type: 'boolean',
group: SSL_GROUP,
describe: 'HTTP/2, must be used with HTTPS',
},
key: {
type: 'string',
describe: 'Path to a SSL key.',
Expand Down
33 changes: 30 additions & 3 deletions lib/Server.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,13 +89,19 @@ class Server {
if (options.lazy && !options.filename) {
throw new Error("'filename' option must be set in lazy mode.");
}


// if the user enables http2, we can safely enable https
if (options.http2 && !options.https) {
options.https = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's separate spdy and http2 options, because it is misleading, spdy and https2 are difference protocols, better don't mix their

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@evilebottnawi I considered http2 an alias for spdy because http2 needs spdy to work at the moment. As an alternative, I could do this, to allow for future separation of http2 from spdy:

When ONLY http2 is enabled, the spdy server will be run (since this is the only way to run HTTP/2 on express at the moment), and these protocols will be provided to spdy:

options.https.spdy = {
  protocols: ['h2', 'http/1.1'],
};

Then the exact same thing will happen as above if http2 is enabled and spdy is enabled.

If ONLY spdy is enabled, the options.https.spdy will not be set unless the user specifies it themselves. If the user does not specify it in their options, it will go to the default spdy value as specified here:

protocols: ['h2','spdy/3.1', 'spdy/3', 'spdy/2','http/1.1', 'http/1.0']

Do you like this alternative better?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's use the http2 option instead spdy. And we can introduce spdy under other option in future because spdy !== http2, also we need throw error if developer doesn't defined https when use http2: true

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, that makes sense. Should user still be allowed to provide spdy options like this?:

{
  contentBase: './dist',
  watchContentBase: true,
  https: {
    spdy: {
      protocols: ['h2', 'http/1.1' ...]
    }
  },
  http2: true
}

Copy link
Member

Choose a reason for hiding this comment

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

@Loonride i thought about it, let's do this in other PR

Choose a reason for hiding this comment

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

@evilebottnawi i think it would help if the http2 option was nested under https because it makes it more obvious that http2 requires https. Perhaps even when only

{
    https: {
        http2: true,
    },
}

Treat that as if both https (with self-signed cert) & http2 are enabled?

Copy link
Member

Choose a reason for hiding this comment

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

hm, http2 !== https2, but no one browsers don't support http2 without https, but for avoid misleading better contain 2 option (i think), new developers can start think what http2 === https, but i am open for feedback

Choose a reason for hiding this comment

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

Actually, I like your suggestion below: assume https: true when http2: true and no https config is specified (and maybe output something in verbose logging about the assumption).

Choose a reason for hiding this comment

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

Oh, duh. That's exactly what this code is doing 🤦‍♂️


updateCompiler(compiler, options);

this.stats =
options.stats && Object.keys(options.stats).length
? options.stats
: Server.DEFAULT_STATS;

this.hot = options.hot || options.hotOnly;
this.headers = options.headers;
this.progress = options.progress;
Expand Down Expand Up @@ -647,7 +653,21 @@ class Server {
options.https.key = options.https.key || fakeCert;
options.https.cert = options.https.cert || fakeCert;

if (!options.https.spdy) {
// Only prevent HTTP/2 if http2 is explicitly set to false
const isHttp2 = options.http2 !== false;

// note that options.spdy never existed. The user was able
// to set options.https.spdy before, though it was not in the
// docs. Keep options.https.spdy if the user sets it for
// backwards compatability, but log a deprecation warning.
if (options.https.spdy) {
// for backwards compatability: if options.https.spdy was passed in before,
// it was not altered in any way
this.log.warn(
'Providing custom spdy server options is deprecated and will be removed in the next major version.'
);
} else {
Copy link
Member

Choose a reason for hiding this comment

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

spdy should enable h2 (don't do braking change, even when option doesn't exists in docs)

Deprecation message is ok 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is not a breaking change, if options.https.spdy is set by the user, it will not be changed at all currently: https://github.com/webpack/webpack-dev-server/blob/master/lib/Server.js#L650

// if the normal https server gets this option, it will not affect it.
options.https.spdy = {
protocols: ['h2', 'http/1.1'],
};
Expand All @@ -662,7 +682,14 @@ class Server {
// - https://github.com/nodejs/node/issues/21665
// - https://github.com/webpack/webpack-dev-server/issues/1449
// - https://github.com/expressjs/express/issues/3388
if (semver.gte(process.version, '10.0.0')) {
if (semver.gte(process.version, '10.0.0') || !isHttp2) {
if (options.http2) {
// the user explicitly requested http2 but is not getting it because
// of the node version.
this.log.warn(
'HTTP/2 is currently unsupported for Node 10.0.0 and above, but will be supported once Express supports it'
);
}
this.listeningApp = https.createServer(options.https, app);
} else {
/* eslint-disable global-require */
Expand Down
4 changes: 4 additions & 0 deletions lib/options.json
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,9 @@
}
]
},
"http2": {
Copy link
Member

Choose a reason for hiding this comment

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

Don't remove spdy, we just new output deprecation as i written above

"type": "boolean"
},
"contentBase": {
"anyOf": [
{
Expand Down Expand Up @@ -321,6 +324,7 @@
"disableHostCheck": "should be {Boolean} (https://webpack.js.org/configuration/dev-server/#devserver-disablehostcheck)",
"public": "should be {String} (https://webpack.js.org/configuration/dev-server/#devserver-public)",
"https": "should be {Object|Boolean} (https://webpack.js.org/configuration/dev-server/#devserver-https)",
"http2": "should be {Boolean} (https://webpack.js.org/configuration/dev-server/#devserver-http2)",
"contentBase": "should be {Array} (https://webpack.js.org/configuration/dev-server/#devserver-contentbase)",
Copy link
Member

Choose a reason for hiding this comment

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

Don't remove, also we need create issue about documentation this (not related to PR right now, we should do this after merge)

"watchContentBase": "should be {Boolean} (https://webpack.js.org/configuration/dev-server/#devserver-watchcontentbase)",
"open": "should be {String|Boolean} (https://webpack.js.org/configuration/dev-server/#devserver-open)",
Expand Down
4 changes: 4 additions & 0 deletions lib/utils/createConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,10 @@ function createConfig(config, argv, { port }) {
options.https = true;
}

if (argv.http2) {
options.http2 = true;
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

Choose a reason for hiding this comment

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

Suggestion for a later/separate cleanup: There are a lot of if (argv.foo) options.foo = true. For the ones that are a simple copy:

const simpleOverrides = _.pick(argv, [
  'http2',
  // other simple if argv then set options
]);

_.merge(options, simpleOverrides);

}

if (argv.key) {
options.key = argv.key;
}
Expand Down
41 changes: 11 additions & 30 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 22 additions & 0 deletions test/CreateConfig.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -560,6 +560,28 @@ describe('createConfig', () => {
expect(config).toMatchSnapshot();
});

it('http2 option', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Don't remove this test, just move this in Http2 test

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this would stay in CreateConfig.test.js since it is only confirming that the config option appears after passing through the config helper, right? Http2 test should be for actual functionality of the server, not just confirming that the config options get through to the server

Copy link
Member

Choose a reason for hiding this comment

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

👍

const config = createConfig(
webpackConfig,
Object.assign({}, argv, { https: true, http2: true }),
{ port: 8080 }
);

expect(config).toMatchSnapshot();
});

it('http2 option (in devServer config)', () => {
const config = createConfig(
Object.assign({}, webpackConfig, {
devServer: { https: true, http2: true },
}),
argv,
{ port: 8080 }
);

expect(config).toMatchSnapshot();
});

it('key option', () => {
const config = createConfig(
webpackConfig,
Expand Down
Loading