-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New spdy option to enable/disable HTTP/2 with HTTPS #1721
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
Changes from 6 commits
58ffad8
af6fe30
1f55c14
3049888
d97ed54
7884dc8
9167003
21afdb0
a79c362
76711b8
9afb2b9
d05a3e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -87,6 +87,10 @@ class Server { | |
throw new Error("'filename' option must be set in lazy mode."); | ||
} | ||
|
||
if (options.http2 && !options.https) { | ||
throw new Error("'https' option must be enabled to use HTTP/2"); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's separate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @evilebottnawi I considered When ONLY options.https.spdy = {
protocols: ['h2', 'http/1.1'],
}; Then the exact same thing will happen as above if If ONLY protocols: ['h2','spdy/3.1', 'spdy/3', 'spdy/2','http/1.1', 'http/1.0'] Do you like this alternative better? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, let's use the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Loonride i thought about it, let's do this in other PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @evilebottnawi i think it would help if the {
https: {
http2: true,
},
} Treat that as if both https (with self-signed cert) & http2 are enabled? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I like your suggestion below: assume There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -630,12 +634,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. | ||
|
@@ -645,9 +643,12 @@ 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.http2) { | ||
this.listeningApp = https.createServer(options.https, app); | ||
} else { | ||
options.https.spdy = { | ||
protocols: ['h2', 'http/1.1'], | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately, it is breaking change, we can't remove
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @evilebottnawi I added the if (!options.https.spdy) {
options.https.spdy = {
protocols: ['h2', 'http/1.1'],
};
}
if (semver.gte(process.version, '10.0.0')) {
this.listeningApp = https.createServer(options.https, app);
} else {
/* eslint-disable global-require */
// The relevant issues are:
// https://github.com/spdy-http2/node-spdy/issues/350
// https://github.com/webpack/webpack-dev-server/issues/1592
this.listeningApp = require('spdy').createServer(options.https, app);
/* eslint-enable global-require */
}
} else {
this.listeningApp = http.createServer(app);
} Actually to avoid breaking changes, I think it should be like this: const isHttp2 = options.http2 || options.https.spdy;
if (options.https.spdy) {
// log deprecation warning, this means they provided it as part of https option
}
if (semver.gte(process.version, '10.0.0') || !isHttp2) {
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
// https://github.com/webpack/webpack-dev-server/issues/1592
this.listeningApp = require('spdy').createServer(options.https, app);
/* eslint-enable global-require */
} So basically, user should still be allowed to use this: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
/* eslint-disable global-require */ | ||
// The relevant issues are: | ||
// https://github.com/spdy-http2/node-spdy/issues/350 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -162,6 +162,9 @@ | |
} | ||
] | ||
}, | ||
"http2": { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't remove |
||
"type": "boolean" | ||
}, | ||
"contentBase": { | ||
"anyOf": [ | ||
{ | ||
|
@@ -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)", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -136,6 +136,10 @@ function createConfig(config, argv, { port }) { | |
options.https = true; | ||
} | ||
|
||
if (argv.http2) { | ||
options.http2 = true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion for a later/separate cleanup: There are a lot of const simpleOverrides = _.pick(argv, [
'http2',
// other simple if argv then set options
]);
_.merge(options, simpleOverrides); |
||
} | ||
|
||
if (argv.key) { | ||
options.key = argv.key; | ||
} | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -559,6 +559,28 @@ describe('createConfig', () => { | |
expect(config).toMatchSnapshot(); | ||
}); | ||
|
||
it('http2 option', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't remove this test, just move this in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this would stay in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
'use strict'; | ||
|
||
const path = require('path'); | ||
const http2 = require('http2'); | ||
const request = require('supertest'); | ||
const semver = require('semver'); | ||
const helper = require('./helper'); | ||
const config = require('./fixtures/contentbase-config/webpack.config'); | ||
|
||
const contentBasePublic = path.join( | ||
__dirname, | ||
'fixtures/contentbase-config/public' | ||
); | ||
|
||
describe('HTTP2', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
let server; | ||
let req; | ||
|
||
describe('without https it should throw an error', () => { | ||
it('create server', () => { | ||
expect(() => { | ||
helper.start(config, { | ||
http2: true, | ||
}); | ||
}).toThrow(/'https' option must be enabled/); | ||
}); | ||
}); | ||
|
||
// HTTP/2 will only work with node versions below 10.0.0 | ||
// since spdy is broken past that point | ||
if (semver.lt(process.version, '10.0.0')) { | ||
describe('http2 works with https', () => { | ||
beforeAll((done) => { | ||
server = helper.start( | ||
config, | ||
{ | ||
contentBase: contentBasePublic, | ||
https: true, | ||
http2: true, | ||
}, | ||
done | ||
); | ||
req = request(server.app); | ||
}); | ||
|
||
it('confirm http2 client can connect', (done) => { | ||
const client = http2.connect('https://localhost:8080', { | ||
rejectUnauthorized: false, | ||
}); | ||
client.on('error', (err) => console.error(err)); | ||
|
||
const http2Req = client.request({ ':path': '/' }); | ||
|
||
http2Req.on('response', (headers) => { | ||
expect(headers[':status']).toEqual(200); | ||
}); | ||
|
||
http2Req.setEncoding('utf8'); | ||
let data = ''; | ||
http2Req.on('data', (chunk) => { | ||
data += chunk; | ||
}); | ||
http2Req.on('end', () => { | ||
expect(data).toEqual(expect.stringMatching(/Heyo/)); | ||
done(); | ||
}); | ||
http2Req.end(); | ||
}); | ||
|
||
afterAll(helper.close); | ||
}); | ||
} | ||
|
||
describe('https without http2 disables HTTP/2', () => { | ||
beforeAll((done) => { | ||
server = helper.start( | ||
config, | ||
{ | ||
contentBase: contentBasePublic, | ||
https: true, | ||
}, | ||
done | ||
); | ||
req = request(server.app); | ||
}); | ||
|
||
it('Request to index', (done) => { | ||
req | ||
.get('/') | ||
.expect(200, /Heyo/) | ||
.then(({ res }) => { | ||
expect(res.httpVersion).not.toEqual('2.0'); | ||
done(); | ||
}); | ||
}); | ||
|
||
afterAll(helper.close); | ||
}); | ||
}); |
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 was wrong, it is poor DX, better set
https: true
in this case, no one browsers don't supporthttp2
withoutssl
so, we can generatecertificate
sefely