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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 12 commits into from
Apr 4, 2019
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
58ffad8
added spdy option with http2 alias
knagaitsev Mar 16, 2019
af6fe30
fixed error message types for spdy and http2
knagaitsev Mar 16, 2019
1f55c14
added docs url
knagaitsev Mar 16, 2019
3049888
removed unused variables in spdy test
knagaitsev Mar 16, 2019
d97ed54
made it possible to provide your own spdy protocols, with test
knagaitsev Mar 16, 2019
7884dc8
removed spdy option in favor of only http2 option, added test using n…
knagaitsev Mar 19, 2019
9167003
deprecation warning to not use https.spdy option, adjust http2 test t…
knagaitsev Mar 21, 2019
21afdb0
slightly better backwards compatability with https.spdy deprecated op…
knagaitsev Mar 21, 2019
a79c362
default to http2 unless it is explicitly set to false, add more warnings
knagaitsev Mar 30, 2019
76711b8
Merge branch 'master' into http2-spdy
knagaitsev Mar 30, 2019
9afb2b9
Merge branch 'master' into http2-spdy
evilebottnawi Apr 3, 2019
d05a3e5
Merge branch 'master' into http2-spdy
evilebottnawi Apr 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions bin/options.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,12 @@ const options = {
group: SSL_GROUP,
describe: 'HTTPS',
},
spdy: {
type: 'boolean',
alias: 'http2',
group: SSL_GROUP,
describe: 'HTTP/2 using spdy',
},
key: {
type: 'string',
describe: 'Path to a SSL key.',
Expand Down
18 changes: 11 additions & 7 deletions lib/Server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,11 @@ class Server {
throw new Error("'filename' option must be set in lazy mode.");
}

if (options.spdy || options.http2) {
options.spdy = true;
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.

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

Copy link

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

Copy link

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).

Copy link

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 🤦‍♂️


this.hot = options.hot || options.hotOnly;
this.headers = options.headers;
this.progress = options.progress;
Expand Down Expand Up @@ -630,12 +635,6 @@ class Server {
options.https.key = options.https.key || fakeCert;
options.https.cert = options.https.cert || fakeCert;

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

// `spdy` is effectively unmaintained, and as a consequence of an
// implementation that extensively relies on Node’s non-public APIs, broken
// on Node 10 and above. In those cases, only https will be used for now.
Expand All @@ -645,9 +644,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') || !options.spdy) {
Copy link
Member

Choose a reason for hiding this comment

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

I think !options.http2 should be added here.

this.listeningApp = https.createServer(options.https, app);
} else {
if (!options.https.spdy) {
options.https.spdy = {
protocols: ['h2', 'http/1.1'],
};
}
/* eslint-disable global-require */
// The relevant issues are:
// https://github.com/spdy-http2/node-spdy/issues/350
Expand Down
8 changes: 8 additions & 0 deletions lib/options.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,12 @@
}
]
},
"spdy": {
"type": "boolean"
},
"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 +327,8 @@
"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)",
"spdy": "should be {Boolean} (https://webpack.js.org/configuration/dev-server/#devserver-spdy)",
"http2": "should be {Boolean} (https://webpack.js.org/configuration/dev-server/#devserver-spdy)",
"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
View file
Open in desktop
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.spdy) {
options.spdy = true;
}

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

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

44 changes: 44 additions & 0 deletions test/CreateConfig.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,6 +559,50 @@ describe('createConfig', () => {
expect(config).toMatchSnapshot();
});

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

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

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

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