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.

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

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

Merged
merged 12 commits into from
Apr 4, 2019
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
5 changes: 5 additions & 0 deletions bin/options.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,11 @@ const options = {
group: SSL_GROUP,
describe: 'HTTPS',
},
http2: {
type: 'boolean',
group: SSL_GROUP,
describe: 'HTTP/2, must be used with HTTPS',
},
key: {
type: 'string',
describe: 'Path to a SSL key.',
Expand Down
15 changes: 8 additions & 7 deletions lib/Server.js
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Copy link
Member

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 support http2 without ssl so, we can generate certificate sefely

}
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

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

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

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 +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.
Expand All @@ -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'],
};
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, it is breaking change, we can't remove spdy option in this version, spdy, how we should rewrite this part:

const isHttp2 = options.http2 || options.spdy
// Log deprecation `spdy` option.

if (semver.gte(process.version, '10.0.0') || !isHttp2) {
        this.listeningApp = https.createServer(options.https, app);
      } else {
        options.https.spdy = {
          protocols: ['http/1.1'],
        };

        // Allow to using https with `http1.1`
        if (isHttp2) { 
          options.https.spdy.protocols.unshift('h2');
        }

        /* 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 */
      }

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 added the spdy option earlier in this PR, it wouldn't be a breaking change to remove it. Currently it looks like this in the code:

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: options.https.spdy. It was never in the docs, but it was possible to use it before. The only breaking change now would be that they used to get http2 automatically if they were below Node 10.0.0. Now, they have to set http2 to true to get http2.

Copy link
Member

Choose a reason for hiding this comment

The 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
Expand Down
4 changes: 4 additions & 0 deletions lib/options.json
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,9 @@
}
]
},
"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 +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)",
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
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.http2) {
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.

Same as above

Choose a reason for hiding this comment

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

Suggestion for a later/separate cleanup: There are a lot of if (argv.foo) options.foo = true. For the ones that are a simple copy:

const simpleOverrides = _.pick(argv, [
  'http2',
  // other simple if argv then set options
]);

_.merge(options, simpleOverrides);

}

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

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

22 changes: 22 additions & 0 deletions test/CreateConfig.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -559,6 +559,28 @@ describe('createConfig', () => {
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
99 changes: 99 additions & 0 deletions test/Http2.test.js
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', () => {
Copy link
Member

Choose a reason for hiding this comment

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

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);
});
});