From 89609ea82f16635f2d47361ff3fb69d0f20ea143 Mon Sep 17 00:00:00 2001 From: Rohan Bhanderi Date: Wed, 31 Oct 2018 07:54:58 -0700 Subject: [PATCH] Fix server: don't use spdy on node >= v10.0.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ports fix for https://github.com/webpack/webpack-dev-server/issues/1449 to v2 branch cherry pick of https://github.com/webpack/webpack-dev-server/pull/1451/commits/e97d345ac370095a6e339b7997b939c88ef3e81b this issue was fixed in webpack-dev-server v3 line, which requires webpack to be upgraded to >= 4.0.0. This commit cherry picks the fix to v2 branch (on top of v2.11.3) which does not require webpack to be upgraded to 4.0.0 `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. Once express supports Node's built-in HTTP/2 support, migrating over to that should be the best way to go. related issues: nodejs/node#21665 https://github.com/nodejs/node/issues/21665 https://github.com/webpack/webpack-dev-server/issues/1449 https://github.com/expressjs/express/issues/3388 --- lib/Server.js | 16 +++++++++++++++- test/Https.test.js | 27 +++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 test/Https.test.js diff --git a/lib/Server.js b/lib/Server.js index 3250722f5f..d6fac39683 100644 --- a/lib/Server.js +++ b/lib/Server.js @@ -18,6 +18,7 @@ const serveIndex = require('serve-index'); const historyApiFallback = require('connect-history-api-fallback'); const selfsigned = require('selfsigned'); const sockjs = require('sockjs'); +const https = require('https'); const spdy = require('spdy'); const webpack = require('webpack'); const webpackDevMiddleware = require('webpack-dev-middleware'); @@ -481,7 +482,20 @@ function Server(compiler, options) { }; } - this.listeningApp = spdy.createServer(options.https, app); + // `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. + // Once express supports Node's built-in HTTP/2 support, migrating over to + // that should be the best way to go. + // The relevant issues are: + // - https://github.com/nodejs/node/issues/21665 + // - https://github.com/webpack/webpack-dev-server/issues/1449 + // - https://github.com/expressjs/express/issues/3388 + if (+process.version.match(/^v(\d+)/)[1] >= 10) { + this.listeningApp = https.createServer(options.https, app); + } else { + this.listeningApp = spdy.createServer(options.https, app); + } } else { this.listeningApp = http.createServer(app); } diff --git a/test/Https.test.js b/test/Https.test.js new file mode 100644 index 0000000000..f97593b65f --- /dev/null +++ b/test/Https.test.js @@ -0,0 +1,27 @@ +'use strict'; + const path = require('path'); +const request = require('supertest'); +const helper = require('./helper'); +const config = require('./fixtures/contentbase-config/webpack.config'); +require('mocha-sinon'); + const contentBasePublic = path.join(__dirname, 'fixtures/contentbase-config/public'); + describe('HTTPS', function testHttps() { + let server; + let req; + afterEach(helper.close); + // Increase the timeout to 20 seconds to allow time for key generation. + this.timeout(20000); + describe('to directory', () => { + before((done) => { + server = helper.start(config, { + contentBase: contentBasePublic, + https: true + }, done); + req = request(server.app); + }); + it('Request to index', (done) => { + req.get('/') + .expect(200, /Heyo/, done); + }); + }); +});