Skip to content

BREAKING CHANGE(server): get rid of conditions of Node version #2552

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

Merged
merged 1 commit into from
Apr 30, 2020
Merged

BREAKING CHANGE(server): get rid of conditions of Node version #2552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
40 changes: 4 additions & 36 deletions lib/Server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,10 @@
*/
const fs = require('fs');
const path = require('path');
const tls = require('tls');
const url = require('url');
const http = require('http');
const https = require('https');
const ip = require('ip');
const semver = require('semver');
const killable = require('killable');
const chokidar = require('chokidar');
const express = require('express');
Expand All @@ -35,15 +33,6 @@ const routes = require('./utils/routes');
const getSocketServerImplementation = require('./utils/getSocketServerImplementation');
const schema = require('./options.json');

// Workaround for node ^8.6.0, ^9.0.0
// DEFAULT_ECDH_CURVE is default to prime256v1 in these version
// breaking connection when certificate is not signed with prime256v1
// change it to auto allows OpenSSL to select the curve automatically
// See https://github.com/nodejs/node/issues/16196 for more information
if (semver.satisfies(process.version, '8.6.0 - 9')) {
tls.DEFAULT_ECDH_CURVE = 'auto';
}

if (!process.env.WEBPACK_DEV_SERVER) {
process.env.WEBPACK_DEV_SERVER = true;
}
Expand Down Expand Up @@ -656,35 +645,14 @@ class Server {

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

// `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 (semver.gte(process.version, '10.0.0') || !isHttp2) {
if (this.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(this.options.https, this.app);
} else {
// The relevant issues are:
// https://github.com/spdy-http2/node-spdy/issues/350
// https://github.com/webpack/webpack-dev-server/issues/1592
if (this.options.http2) {
// TODO: we need to replace spdy with http2 which is an internal module
Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally, we wanna use http2 which is an internal module but we don't have enough time to implement because the interface isn't the same, so we use spdy for all Node versions because spdy supports 10, 12, and 14.

this.listeningApp = require('spdy').createServer(
this.options.https,
this.app
);
} else {
this.listeningApp = https.createServer(this.options.https, this.app);
}
} else {
this.listeningApp = http.createServer(this.app);
Expand Down
5 changes: 0 additions & 5 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.

3 changes: 1 addition & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
"client"
],
"engines": {
"node": ">= 6.11.5"
"node": ">= 10.13.0"
},
"scripts": {
"lint:prettier": "prettier \"{**/*,*}.{js,json,md,yml,css}\" --list-different",
Expand Down Expand Up @@ -57,7 +57,6 @@
"portfinder": "^1.0.25",
"schema-utils": "^2.6.6",
"selfsigned": "^1.10.7",
"semver": "^7.3.2",
"serve-index": "^1.9.1",
"sockjs": "0.3.20",
"sockjs-client": "1.4.0",
Expand Down
85 changes: 36 additions & 49 deletions test/server/http2-option.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
'use strict';

const path = require('path');
const http2 = require('http2');
const request = require('supertest');
const semver = require('semver');
const testServer = require('../helpers/test-server');
const config = require('../fixtures/contentbase-config/webpack.config');
const port = require('../ports-map')['http2-option'];
Expand All @@ -16,60 +16,47 @@ describe('http2 option', () => {
let server;
let req;

// HTTP/2 will only work with node versions below 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')
) {
const http2 = require('http2');
describe('http2 works with https', () => {
beforeAll((done) => {
server = testServer.start(
config,
{
contentBase: contentBasePublic,
https: true,
http2: true,
port,
},
done
);
req = request(server.app);
});

it('confirm http2 client can connect', (done) => {
const client = http2.connect(`https://localhost:${port}`, {
rejectUnauthorized: false,
});
client.on('error', (err) => console.error(err));
describe('http2 works with https', () => {
beforeAll((done) => {
server = testServer.start(
config,
{
contentBase: contentBasePublic,
https: true,
http2: true,
port,
},
done
);
req = request(server.app);
});

const http2Req = client.request({ ':path': '/' });
it('confirm http2 client can connect', (done) => {
const client = http2.connect(`https://localhost:${port}`, {
rejectUnauthorized: false,
});
client.on('error', (err) => console.error(err));

http2Req.on('response', (headers) => {
expect(headers[':status']).toEqual(200);
});
const http2Req = client.request({ ':path': '/' });

http2Req.setEncoding('utf8');
let data = '';
http2Req.on('data', (chunk) => {
data += chunk;
});
http2Req.on('end', () => {
expect(data).toEqual(expect.stringMatching(/Heyo/));
done();
});
http2Req.end();
http2Req.on('response', (headers) => {
expect(headers[':status']).toEqual(200);
});

afterAll(testServer.close);
http2Req.setEncoding('utf8');
let data = '';
http2Req.on('data', (chunk) => {
data += chunk;
});
http2Req.on('end', () => {
expect(data).toEqual(expect.stringMatching(/Heyo/));
done();
});
http2Req.end();
});
}

afterAll(testServer.close);
});

describe('server works with http2 option, but without https option', () => {
beforeAll((done) => {
Expand Down