From 58ffad896352b96acea904e710c2899d05eacf6c Mon Sep 17 00:00:00 2001 From: Kirill Nagaitsev Date: Sat, 16 Mar 2019 12:44:18 -0500 Subject: [PATCH 1/9] added spdy option with http2 alias --- bin/options.js | 6 ++++++ lib/Server.js | 16 ++++++++------- lib/options.json | 8 ++++++++ lib/utils/createConfig.js | 4 ++++ package-lock.json | 41 +++++++++++---------------------------- 5 files changed, 38 insertions(+), 37 deletions(-) diff --git a/bin/options.js b/bin/options.js index 35db9b3158..87fb041455 100644 --- a/bin/options.js +++ b/bin/options.js @@ -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.', diff --git a/lib/Server.js b/lib/Server.js index 07c19ff54c..c6209e45bf 100644 --- a/lib/Server.js +++ b/lib/Server.js @@ -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; + } + this.hot = options.hot || options.hotOnly; this.headers = options.headers; this.progress = options.progress; @@ -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. @@ -645,9 +644,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.spdy) { this.listeningApp = https.createServer(options.https, app); } else { + 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 diff --git a/lib/options.json b/lib/options.json index 7e4e2b3db9..65b7dda1da 100644 --- a/lib/options.json +++ b/lib/options.json @@ -162,6 +162,12 @@ } ] }, + "spdy": { + "type": "boolean" + }, + "http2": { + "type": "boolean" + }, "contentBase": { "anyOf": [ { @@ -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": "test", + "http2": "test", "contentBase": "should be {Array} (https://webpack.js.org/configuration/dev-server/#devserver-contentbase)", "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)", diff --git a/lib/utils/createConfig.js b/lib/utils/createConfig.js index 53e00cf2af..6da3e3a7dd 100644 --- a/lib/utils/createConfig.js +++ b/lib/utils/createConfig.js @@ -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; } diff --git a/package-lock.json b/package-lock.json index ee87d059ce..eb33db2233 100644 --- a/package-lock.json +++ b/package-lock.json @@ -4674,8 +4674,7 @@ }, "ansi-regex": { "version": "2.1.1", - "bundled": true, - "optional": true + "bundled": true }, "aproba": { "version": "1.2.0", @@ -4693,13 +4692,11 @@ }, "balanced-match": { "version": "1.0.0", - "bundled": true, - "optional": true + "bundled": true }, "brace-expansion": { "version": "1.1.11", "bundled": true, - "optional": true, "requires": { "balanced-match": "^1.0.0", "concat-map": "0.0.1" @@ -4712,18 +4709,15 @@ }, "code-point-at": { "version": "1.1.0", - "bundled": true, - "optional": true + "bundled": true }, "concat-map": { "version": "0.0.1", - "bundled": true, - "optional": true + "bundled": true }, "console-control-strings": { "version": "1.1.0", - "bundled": true, - "optional": true + "bundled": true }, "core-util-is": { "version": "1.0.2", @@ -4826,8 +4820,7 @@ }, "inherits": { "version": "2.0.3", - "bundled": true, - "optional": true + "bundled": true }, "ini": { "version": "1.3.5", @@ -4837,7 +4830,6 @@ "is-fullwidth-code-point": { "version": "1.0.0", "bundled": true, - "optional": true, "requires": { "number-is-nan": "^1.0.0" } @@ -4850,20 +4842,17 @@ "minimatch": { "version": "3.0.4", "bundled": true, - "optional": true, "requires": { "brace-expansion": "^1.1.7" } }, "minimist": { "version": "0.0.8", - "bundled": true, - "optional": true + "bundled": true }, "minipass": { "version": "2.3.5", "bundled": true, - "optional": true, "requires": { "safe-buffer": "^5.1.2", "yallist": "^3.0.0" @@ -4880,7 +4869,6 @@ "mkdirp": { "version": "0.5.1", "bundled": true, - "optional": true, "requires": { "minimist": "0.0.8" } @@ -4953,8 +4941,7 @@ }, "number-is-nan": { "version": "1.0.1", - "bundled": true, - "optional": true + "bundled": true }, "object-assign": { "version": "4.1.1", @@ -4964,7 +4951,6 @@ "once": { "version": "1.4.0", "bundled": true, - "optional": true, "requires": { "wrappy": "1" } @@ -5040,8 +5026,7 @@ }, "safe-buffer": { "version": "5.1.2", - "bundled": true, - "optional": true + "bundled": true }, "safer-buffer": { "version": "2.1.2", @@ -5071,7 +5056,6 @@ "string-width": { "version": "1.0.2", "bundled": true, - "optional": true, "requires": { "code-point-at": "^1.0.0", "is-fullwidth-code-point": "^1.0.0", @@ -5089,7 +5073,6 @@ "strip-ansi": { "version": "3.0.1", "bundled": true, - "optional": true, "requires": { "ansi-regex": "^2.0.0" } @@ -5128,13 +5111,11 @@ }, "wrappy": { "version": "1.0.2", - "bundled": true, - "optional": true + "bundled": true }, "yallist": { "version": "3.0.3", - "bundled": true, - "optional": true + "bundled": true } } }, From af6fe3039e42b99878f28f4cf98174f6a889875f Mon Sep 17 00:00:00 2001 From: Kirill Nagaitsev Date: Sat, 16 Mar 2019 12:51:50 -0500 Subject: [PATCH 2/9] fixed error message types for spdy and http2 --- lib/options.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/options.json b/lib/options.json index 65b7dda1da..612749caff 100644 --- a/lib/options.json +++ b/lib/options.json @@ -327,8 +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": "test", - "http2": "test", + "spdy": "should be {Boolean}", + "http2": "should be {Boolean}", "contentBase": "should be {Array} (https://webpack.js.org/configuration/dev-server/#devserver-contentbase)", "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)", From 1f55c145d26169268bfe69fd4d11d98167aea9d1 Mon Sep 17 00:00:00 2001 From: Kirill Nagaitsev Date: Sat, 16 Mar 2019 15:06:44 -0500 Subject: [PATCH 3/9] added docs url --- lib/options.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/options.json b/lib/options.json index 612749caff..f19cb91bc1 100644 --- a/lib/options.json +++ b/lib/options.json @@ -327,8 +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}", - "http2": "should be {Boolean}", + "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)", "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)", From 304988808699042eb16bb37e48609ec08eb881e2 Mon Sep 17 00:00:00 2001 From: Kirill Nagaitsev Date: Sat, 16 Mar 2019 16:08:48 -0500 Subject: [PATCH 4/9] removed unused variables in spdy test --- test/CreateConfig.test.js | 44 ++++++++++ test/Spdy.test.js | 86 ++++++++++++++++++++ test/__snapshots__/CreateConfig.test.js.snap | 63 ++++++++++++++ 3 files changed, 193 insertions(+) create mode 100644 test/Spdy.test.js diff --git a/test/CreateConfig.test.js b/test/CreateConfig.test.js index edeb0da422..011d8f0178 100644 --- a/test/CreateConfig.test.js +++ b/test/CreateConfig.test.js @@ -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', () => { + 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, diff --git a/test/Spdy.test.js b/test/Spdy.test.js new file mode 100644 index 0000000000..bb734a16f6 --- /dev/null +++ b/test/Spdy.test.js @@ -0,0 +1,86 @@ +'use strict'; + +const path = require('path'); +const request = require('supertest'); +const helper = require('./helper'); +const config = require('./fixtures/contentbase-config/webpack.config'); + +const contentBasePublic = path.join( + __dirname, + 'fixtures/contentbase-config/public' +); + +describe('SPDY', () => { + let server; + let req; + + describe('spdy works with https', () => { + beforeAll((done) => { + server = helper.start( + config, + { + contentBase: contentBasePublic, + https: true, + spdy: true, + }, + done + ); + req = request(server.app); + }); + + it('Request to index', (done) => { + req.get('/').expect(200, /Heyo/, done); + }); + + afterAll(() => { + helper.close(); + }); + }); + + describe('spdy ignored without https', () => { + beforeAll((done) => { + server = helper.start( + config, + { + contentBase: contentBasePublic, + spdy: true, + }, + done + ); + req = request(server.app); + }); + + it('Request to index', (done) => { + req.get('/').expect(200, /Heyo/, done); + }); + + afterAll(() => { + helper.close(); + }); + }); + + describe('http2 alias for spdy', () => { + beforeAll((done) => { + server = helper.start( + config, + { + contentBase: contentBasePublic, + https: true, + http2: true, + }, + done + ); + req = request(server.app); + }); + + it('Request to index', (done) => { + req.get('/').expect(200, /Heyo/, done); + }); + + afterAll(() => { + helper.close(); + }); + }); + + afterEach(helper.close); +}); diff --git a/test/__snapshots__/CreateConfig.test.js.snap b/test/__snapshots__/CreateConfig.test.js.snap index c66f9ab9de..2d977f8e48 100644 --- a/test/__snapshots__/CreateConfig.test.js.snap +++ b/test/__snapshots__/CreateConfig.test.js.snap @@ -474,6 +474,37 @@ Object { } `; +exports[`createConfig http2 option (in devServer config) 1`] = ` +Object { + "hot": true, + "hotOnly": false, + "http2": true, + "https": true, + "noInfo": true, + "port": 8080, + "publicPath": "/", + "stats": Object { + "cached": false, + "cachedAssets": false, + }, +} +`; + +exports[`createConfig http2 option 1`] = ` +Object { + "hot": true, + "hotOnly": false, + "https": true, + "noInfo": true, + "port": 8080, + "publicPath": "/", + "stats": Object { + "cached": false, + "cachedAssets": false, + }, +} +`; + exports[`createConfig https option (in devServer config) 1`] = ` Object { "hot": true, @@ -1012,6 +1043,38 @@ Object { } `; +exports[`createConfig spdy option (in devServer config) 1`] = ` +Object { + "hot": true, + "hotOnly": false, + "https": true, + "noInfo": true, + "port": 8080, + "publicPath": "/", + "spdy": true, + "stats": Object { + "cached": false, + "cachedAssets": false, + }, +} +`; + +exports[`createConfig spdy option 1`] = ` +Object { + "hot": true, + "hotOnly": false, + "https": true, + "noInfo": true, + "port": 8080, + "publicPath": "/", + "spdy": true, + "stats": Object { + "cached": false, + "cachedAssets": false, + }, +} +`; + exports[`createConfig stats option (colors) 1`] = ` Object { "hot": true, From d97ed54b01915fa5922d36f08e4a59a6bd81dcc8 Mon Sep 17 00:00:00 2001 From: Kirill Nagaitsev Date: Sat, 16 Mar 2019 16:31:56 -0500 Subject: [PATCH 5/9] made it possible to provide your own spdy protocols, with test --- lib/Server.js | 8 +++++--- test/Spdy.test.js | 27 +++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/lib/Server.js b/lib/Server.js index c6209e45bf..e29941aeb5 100644 --- a/lib/Server.js +++ b/lib/Server.js @@ -647,9 +647,11 @@ class Server { if (semver.gte(process.version, '10.0.0') || !options.spdy) { this.listeningApp = https.createServer(options.https, app); } else { - options.https.spdy = { - protocols: ['h2', 'http/1.1'], - }; + 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 diff --git a/test/Spdy.test.js b/test/Spdy.test.js index bb734a16f6..9cfc161b10 100644 --- a/test/Spdy.test.js +++ b/test/Spdy.test.js @@ -82,5 +82,32 @@ describe('SPDY', () => { }); }); + describe('custom spdy protocol options', () => { + beforeAll((done) => { + server = helper.start( + config, + { + contentBase: contentBasePublic, + https: { + spdy: { + protocols: ['http/1.1'], + }, + }, + spdy: true, + }, + done + ); + req = request(server.app); + }); + + it('Request to index', (done) => { + req.get('/').expect(200, /Heyo/, done); + }); + + afterAll(() => { + helper.close(); + }); + }); + afterEach(helper.close); }); From 7884dc80641f50357bedf74f2f0922aefab95c10 Mon Sep 17 00:00:00 2001 From: Kirill Nagaitsev Date: Tue, 19 Mar 2019 17:58:47 -0500 Subject: [PATCH 6/9] removed spdy option in favor of only http2 option, added test using node's http2 module --- bin/options.js | 5 +- lib/Server.js | 15 +-- lib/options.json | 6 +- lib/utils/createConfig.js | 4 +- test/CreateConfig.test.js | 22 ---- test/Http2.test.js | 99 ++++++++++++++++ test/Spdy.test.js | 113 ------------------- test/__snapshots__/CreateConfig.test.js.snap | 33 +----- 8 files changed, 111 insertions(+), 186 deletions(-) create mode 100644 test/Http2.test.js delete mode 100644 test/Spdy.test.js diff --git a/bin/options.js b/bin/options.js index 87fb041455..3670ffe15d 100644 --- a/bin/options.js +++ b/bin/options.js @@ -87,11 +87,10 @@ const options = { group: SSL_GROUP, describe: 'HTTPS', }, - spdy: { + http2: { type: 'boolean', - alias: 'http2', group: SSL_GROUP, - describe: 'HTTP/2 using spdy', + describe: 'HTTP/2, must be used with HTTPS', }, key: { type: 'string', diff --git a/lib/Server.js b/lib/Server.js index e29941aeb5..fff3863c29 100644 --- a/lib/Server.js +++ b/lib/Server.js @@ -87,9 +87,8 @@ class Server { throw new Error("'filename' option must be set in lazy mode."); } - if (options.spdy || options.http2) { - options.spdy = true; - options.http2 = true; + if (options.http2 && !options.https) { + throw new Error("'https' option must be enabled to use HTTP/2"); } this.hot = options.hot || options.hotOnly; @@ -644,14 +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') || !options.spdy) { + if (semver.gte(process.version, '10.0.0') || !options.http2) { this.listeningApp = https.createServer(options.https, app); } else { - if (!options.https.spdy) { - options.https.spdy = { - protocols: ['h2', 'http/1.1'], - }; - } + 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 diff --git a/lib/options.json b/lib/options.json index f19cb91bc1..d3a3bdda62 100644 --- a/lib/options.json +++ b/lib/options.json @@ -162,9 +162,6 @@ } ] }, - "spdy": { - "type": "boolean" - }, "http2": { "type": "boolean" }, @@ -327,8 +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)", - "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)", + "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)", "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)", diff --git a/lib/utils/createConfig.js b/lib/utils/createConfig.js index 6da3e3a7dd..44fd36d07e 100644 --- a/lib/utils/createConfig.js +++ b/lib/utils/createConfig.js @@ -136,8 +136,8 @@ function createConfig(config, argv, { port }) { options.https = true; } - if (argv.spdy) { - options.spdy = true; + if (argv.http2) { + options.http2 = true; } if (argv.key) { diff --git a/test/CreateConfig.test.js b/test/CreateConfig.test.js index 011d8f0178..e04e6dac36 100644 --- a/test/CreateConfig.test.js +++ b/test/CreateConfig.test.js @@ -559,28 +559,6 @@ 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', () => { const config = createConfig( webpackConfig, diff --git a/test/Http2.test.js b/test/Http2.test.js new file mode 100644 index 0000000000..d83f95eb7f --- /dev/null +++ b/test/Http2.test.js @@ -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', () => { + 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); + }); +}); diff --git a/test/Spdy.test.js b/test/Spdy.test.js deleted file mode 100644 index 9cfc161b10..0000000000 --- a/test/Spdy.test.js +++ /dev/null @@ -1,113 +0,0 @@ -'use strict'; - -const path = require('path'); -const request = require('supertest'); -const helper = require('./helper'); -const config = require('./fixtures/contentbase-config/webpack.config'); - -const contentBasePublic = path.join( - __dirname, - 'fixtures/contentbase-config/public' -); - -describe('SPDY', () => { - let server; - let req; - - describe('spdy works with https', () => { - beforeAll((done) => { - server = helper.start( - config, - { - contentBase: contentBasePublic, - https: true, - spdy: true, - }, - done - ); - req = request(server.app); - }); - - it('Request to index', (done) => { - req.get('/').expect(200, /Heyo/, done); - }); - - afterAll(() => { - helper.close(); - }); - }); - - describe('spdy ignored without https', () => { - beforeAll((done) => { - server = helper.start( - config, - { - contentBase: contentBasePublic, - spdy: true, - }, - done - ); - req = request(server.app); - }); - - it('Request to index', (done) => { - req.get('/').expect(200, /Heyo/, done); - }); - - afterAll(() => { - helper.close(); - }); - }); - - describe('http2 alias for spdy', () => { - beforeAll((done) => { - server = helper.start( - config, - { - contentBase: contentBasePublic, - https: true, - http2: true, - }, - done - ); - req = request(server.app); - }); - - it('Request to index', (done) => { - req.get('/').expect(200, /Heyo/, done); - }); - - afterAll(() => { - helper.close(); - }); - }); - - describe('custom spdy protocol options', () => { - beforeAll((done) => { - server = helper.start( - config, - { - contentBase: contentBasePublic, - https: { - spdy: { - protocols: ['http/1.1'], - }, - }, - spdy: true, - }, - done - ); - req = request(server.app); - }); - - it('Request to index', (done) => { - req.get('/').expect(200, /Heyo/, done); - }); - - afterAll(() => { - helper.close(); - }); - }); - - afterEach(helper.close); -}); diff --git a/test/__snapshots__/CreateConfig.test.js.snap b/test/__snapshots__/CreateConfig.test.js.snap index 2d977f8e48..6604aa8d05 100644 --- a/test/__snapshots__/CreateConfig.test.js.snap +++ b/test/__snapshots__/CreateConfig.test.js.snap @@ -494,6 +494,7 @@ exports[`createConfig http2 option 1`] = ` Object { "hot": true, "hotOnly": false, + "http2": true, "https": true, "noInfo": true, "port": 8080, @@ -1043,38 +1044,6 @@ Object { } `; -exports[`createConfig spdy option (in devServer config) 1`] = ` -Object { - "hot": true, - "hotOnly": false, - "https": true, - "noInfo": true, - "port": 8080, - "publicPath": "/", - "spdy": true, - "stats": Object { - "cached": false, - "cachedAssets": false, - }, -} -`; - -exports[`createConfig spdy option 1`] = ` -Object { - "hot": true, - "hotOnly": false, - "https": true, - "noInfo": true, - "port": 8080, - "publicPath": "/", - "spdy": true, - "stats": Object { - "cached": false, - "cachedAssets": false, - }, -} -`; - exports[`createConfig stats option (colors) 1`] = ` Object { "hot": true, From 9167003ef9b45f181f208bdab4332858c8585e73 Mon Sep 17 00:00:00 2001 From: Kirill Nagaitsev Date: Thu, 21 Mar 2019 01:09:33 -0500 Subject: [PATCH 7/9] deprecation warning to not use https.spdy option, adjust http2 test to run only within certain versions --- lib/Server.js | 23 +++++++++++++++++----- test/Http2.test.js | 48 ++++++++++++++++++++++++++++++++-------------- 2 files changed, 52 insertions(+), 19 deletions(-) diff --git a/lib/Server.js b/lib/Server.js index fff3863c29..3505d341e2 100644 --- a/lib/Server.js +++ b/lib/Server.js @@ -87,8 +87,9 @@ class Server { 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) { - throw new Error("'https' option must be enabled to use HTTP/2"); + options.https = true; } this.hot = options.hot || options.hotOnly; @@ -634,6 +635,8 @@ class Server { options.https.key = options.https.key || fakeCert; options.https.cert = options.https.cert || fakeCert; + const isHttp2 = options.http2; + // `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. @@ -643,12 +646,22 @@ 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') || !options.http2) { + if (semver.gte(process.version, '10.0.0') || !isHttp2) { this.listeningApp = https.createServer(options.https, app); } else { - options.https.spdy = { - protocols: ['h2', 'http/1.1'], - }; + // 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) { + this.log.warn( + 'Providing custom spdy server options is deprecated and will be removed in the next major version.' + ); + } else { + 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 diff --git a/test/Http2.test.js b/test/Http2.test.js index d83f95eb7f..720de2f1e8 100644 --- a/test/Http2.test.js +++ b/test/Http2.test.js @@ -1,7 +1,6 @@ 'use strict'; const path = require('path'); -const http2 = require('http2'); const request = require('supertest'); const semver = require('semver'); const helper = require('./helper'); @@ -12,23 +11,24 @@ const contentBasePublic = path.join( 'fixtures/contentbase-config/public' ); -describe('HTTP2', () => { +describe('http2', () => { 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')) { + // since spdy is broken past that point, and this test will only + // work above Node 8.8.0, since it is the first version where the + // built-in http2 module is exposed without need for a flag + // (https://nodejs.org/en/blog/release/v8.8.0/) + // if someone is testing below this Node version and breaks this, + // their tests will not catch it, but CI will catch it. + if ( + semver.gte(process.version, '8.8.0') && + semver.lt(process.version, '10.0.0') + ) { + /* eslint-disable global-require */ + const http2 = require('http2'); + /* eslint-enable global-require */ describe('http2 works with https', () => { beforeAll((done) => { server = helper.start( @@ -71,6 +71,26 @@ describe('HTTP2', () => { }); } + describe('server works with http2 option, but without https option', () => { + beforeAll((done) => { + server = helper.start( + config, + { + contentBase: contentBasePublic, + http2: true, + }, + done + ); + req = request(server.app); + }); + + it('Request to index', (done) => { + req.get('/').expect(200, /Heyo/, done); + }); + + afterAll(helper.close); + }); + describe('https without http2 disables HTTP/2', () => { beforeAll((done) => { server = helper.start( From 21afdb0bdd41a848b37c5eb0e42b66f6af86c28f Mon Sep 17 00:00:00 2001 From: Kirill Nagaitsev Date: Thu, 21 Mar 2019 01:43:30 -0500 Subject: [PATCH 8/9] slightly better backwards compatability with https.spdy deprecated option --- lib/Server.js | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/lib/Server.js b/lib/Server.js index 3505d341e2..8ed0132fb8 100644 --- a/lib/Server.js +++ b/lib/Server.js @@ -635,7 +635,25 @@ class Server { options.https.key = options.https.key || fakeCert; options.https.cert = options.https.cert || fakeCert; - const isHttp2 = options.http2; + // options.https.spdy is here for slightly better backwards compatability, + // since a user will probably provide this deprecated option if they + // are expecting the spdy server to be used. + const isHttp2 = options.http2 || options.https.spdy; + + // 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) { + this.log.warn( + 'Providing custom spdy server options is deprecated and will be removed in the next major version.' + ); + } else { + // if the normal https server gets this option, it will not affect it. + 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 @@ -649,19 +667,6 @@ class Server { if (semver.gte(process.version, '10.0.0') || !isHttp2) { this.listeningApp = https.createServer(options.https, app); } else { - // 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) { - this.log.warn( - 'Providing custom spdy server options is deprecated and will be removed in the next major version.' - ); - } else { - 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 From a79c3626a7e5f16367c99f0bd6f12300e021870d Mon Sep 17 00:00:00 2001 From: Kirill Nagaitsev Date: Sat, 30 Mar 2019 00:31:32 -0500 Subject: [PATCH 9/9] default to http2 unless it is explicitly set to false, add more warnings --- lib/Server.js | 15 +++++++++++---- test/Http2.test.js | 1 + 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/lib/Server.js b/lib/Server.js index 8ed0132fb8..285a99de53 100644 --- a/lib/Server.js +++ b/lib/Server.js @@ -635,16 +635,16 @@ class Server { options.https.key = options.https.key || fakeCert; options.https.cert = options.https.cert || fakeCert; - // options.https.spdy is here for slightly better backwards compatability, - // since a user will probably provide this deprecated option if they - // are expecting the spdy server to be used. - const isHttp2 = options.http2 || 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.' ); @@ -665,6 +665,13 @@ class Server { // - https://github.com/webpack/webpack-dev-server/issues/1449 // - https://github.com/expressjs/express/issues/3388 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 */ diff --git a/test/Http2.test.js b/test/Http2.test.js index 720de2f1e8..d71e352043 100644 --- a/test/Http2.test.js +++ b/test/Http2.test.js @@ -98,6 +98,7 @@ describe('http2', () => { { contentBase: contentBasePublic, https: true, + http2: false, }, done );