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

knagaitsev · 2019-03-16T22:04:10Z

For Bugs and Features; did you add new tests?

Yes

Motivation / Use-Case

I decided to try solving this problem for my first PR: #1713

I added the spdy option for the node API and the CLI, along with an http2 alias option on both which does the same thing. When spdy is enabled, https is enabled, and node version is below 10.0.0 (since spdy is broken beyond that), spdy will be used to create the server.

Breaking Changes

The dev server will no longer automatically serve over HTTP/2 when HTTPS is enabled and Node version is below 10.0.0, as the spdy/http2 option must be true for this to happen.

Additional Info

Changes in docs that I made to reflect the new options: https://github.com/Loonride/webpack.js.org/blob/dev-server-spdy-http2/src/content/configuration/dev-server.md#devserverspdy

codecov · 2019-03-16T22:23:18Z

Codecov Report

Merging #1721 into master will decrease coverage by <.01%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master    #1721      +/-   ##
==========================================
- Coverage   87.54%   87.54%   -0.01%     
==========================================
  Files           9        9              
  Lines         578      586       +8     
  Branches      170      173       +3     
==========================================
+ Hits          506      513       +7     
- Misses         60       61       +1     
  Partials       12       12

Impacted Files	Coverage Δ
lib/utils/createConfig.js	`95.6% <100%> (+0.09%)`	⬆️
lib/Server.js	`83.16% <87.5%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0984d4b...d05a3e5. Read the comment docs.

hiroppy · 2019-03-17T23:54:38Z

lib/Server.js

@@ -645,9 +644,14 @@ 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) {


I think !options.http2 should be added here.

hiroppy · 2019-03-18T00:13:36Z

test/Spdy.test.js

+    });
+
+    it('Request to index', (done) => {
+      req.get('/').expect(200, /Heyo/, done);


Please check http/2 or not.

it('Request to index', (done) => { req .get('/') .expect(200, /Heyo/) .then(({ res }) => { expect(res.httpVersion).toEqual('2.0'); done(); }); });

@hiroppy I've realized this is more difficult than it seems. Supertest sends an HTTP/1.1 request in the test, so the spdy server communicates back with 1.1. I confirmed in the browser that the protocol is h2 and spdy works. The two options I think are: use Node's built-in http2 module for the test and try to communicate with the server using it to confirm HTTP/2 works, or use puppeteer to test with a browser that supports HTTP/2 and confirm that the h2 protocol is used. What do you think?

hiroppy · 2019-03-18T00:15:14Z

test/Spdy.test.js

+    });
+  });
+
+  afterEach(helper.close);


why is this necessary?

Sorry, mistake from copying HTTPS tests over as a template. I will remove it.

alexander-akait

See comment above, thanks!

alexander-akait · 2019-03-18T09:28:41Z

lib/Server.js

+    if (options.spdy || options.http2) {
+      options.spdy = true;
+      options.http2 = true;
+    }


Let's separate spdy and http2 options, because it is misleading, spdy and https2 are difference protocols, better don't mix their

@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?

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

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 }

@Loonride i thought about it, let's do this in other PR

@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?

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

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

Oh, duh. That's exactly what this code is doing 🤦‍♂️

…ode's http2 module

knagaitsev · 2019-03-19T23:11:22Z

I have removed the spdy option and instead just have the http2 option. I also made a test which depends on Node's experimental http2 module, since the only way to test an HTTP/2 server is to make an HTTP/2 request. I thought this was a reasonable choice since it will only force testers to have a Node version high enough for that, not normal users. If you think that is a problem, let me know. My alternative solution would be to dig into puppeteer and find the protocol of the request, similar to the way Chrome DevTools can show the protocol of a request.

alexander-akait

Code looks really good, let's keep compatibility (no breaking change) and improve DX

alexander-akait · 2019-03-20T10:22:48Z

lib/Server.js

-      options.spdy = true;
-      options.http2 = true;
+    if (options.http2 && !options.https) {
+      throw new Error("'https' option must be enabled to use HTTP/2");


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

alexander-akait · 2019-03-20T10:27:59Z

lib/Server.js

-        }
+        options.https.spdy = {
+          protocols: ['h2', 'http/1.1'],
+        };


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

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

alexander-akait · 2019-03-20T10:28:38Z

lib/options.json

@@ -162,9 +162,6 @@
        }
      ]
    },
-    "spdy": {
-      "type": "boolean"
-    },
    "http2": {


Don't remove spdy, we just new output deprecation as i written above

alexander-akait · 2019-03-20T10:29:34Z

lib/options.json

@@ -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)",


Don't remove, also we need create issue about documentation this (not related to PR right now, we should do this after merge)

alexander-akait · 2019-03-20T10:33:29Z

lib/utils/createConfig.js

-  if (argv.spdy) {
-    options.spdy = true;
+  if (argv.http2) {
+    options.http2 = true;


Same as above

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

alexander-akait · 2019-03-20T10:34:07Z

test/CreateConfig.test.js

-
-    expect(config).toMatchSnapshot();
-  });
-
  it('http2 option', () => {


Don't remove this test, just move this in Http2 test

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

alexander-akait · 2019-03-20T10:34:16Z

test/Http2.test.js

+  'fixtures/contentbase-config/public'
+);
+
+describe('HTTP2', () => {


alexander-akait · 2019-03-20T10:34:44Z

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,


Let's add spdy and http2

knagaitsev · 2019-03-20T19:00:58Z

@evilebottnawi It didn't pass CI for Node 6 because I used experimental http2 module for the test added in Node 8. Would it be fine to skip that test below the version where http2 was added, or should I find a new solution to test that HTTP/2 server works?

…o run only within certain versions

…tion

knagaitsev · 2019-03-21T06:50:02Z

Made a few changes. As I said before, options.spdy did not exist before this PR, so I don't think we should add it if it will just instantly be considered deprecated. Instead I think the options.https.spdy should be described as deprecated, even though it was never in the docs.

alexander-akait · 2019-03-29T01:05:09Z

lib/Server.js

+        this.log.warn(
+          'Providing custom spdy server options is deprecated and will be removed in the next major version.'
+        );
+      } else {


spdy should enable h2 (don't do braking change, even when option doesn't exists in docs)

Deprecation message is ok 👍

This is not a breaking change, if options.https.spdy is set by the user, it will not be changed at all currently: https://github.com/webpack/webpack-dev-server/blob/master/lib/Server.js#L650

alexander-akait · 2019-03-29T01:06:06Z

lib/Server.js

+      // 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;


What do you think should we enable http2 be default when user use https: true (without http2 option) for better DX?

I would prefer this since h2 is the standard for most CDNs going forward (or should be). Would be good to mimic the greater movement across the web. I originally opened the issue because I wrongly assumed I was getting h2 by default.

I changed it so that h2 is only prevented if http2 is explicitly set to false.

knagaitsev · 2019-03-30T06:06:34Z

There should be no breaking changes at this point. I added another warning when someone tries to explicitly enable http2, but Node version is 10.0.0 is greater (where it does not work). I'm not sure what the second option is that @evilebottnawi was suggesting above. I think only http2 option should be added for now.

I think what you meant to say earlier is https !== http2. What I interpret as the misleading thing you are mentioning is that if a user wants to use http2, they have to put their key/cert stuff into options.https. Maybe the solution is that http2 should also take an object in the same way that options.https does. But I feel we should hold off on that until Express supports HTTP/2.

JakobJingleheimer · 2019-03-30T10:59:15Z

Mhmm, putting the certs stuff inside http2 would make it obvious that SSL is required, and it align with node's http2:

const server = http2.createSecureServer({
  key: fs.readFileSync('localhost-privkey.pem'),
  cert: fs.readFileSync('localhost-cert.pem')
});

{
  http2: {
    key:  // …
    cert: // …
  }
}

So then https could just relate to http/1.1, and maybe both could have a true option that generates & uses a self-signed cert.

I think it's worth doing now.

But then what if someone includes both?

{
  https: {
    key:  // …
    cert: // …
  },
  http2: true,
}

Throw saying use one or the other? In the above, you could detect that https has certs and use those for http2 instead of generating new ones (and then warn that https and http2 options are mutually exclusive). That sounds like a can of worms though.

alexander-akait

Great work, thanks!

alexander-akait · 2019-04-04T14:50:48Z

/cc @hiroppy

hiroppy

Thanks!

knagaitsev added 5 commits March 16, 2019 12:44

added spdy option with http2 alias

58ffad8

fixed error message types for spdy and http2

af6fe30

added docs url

1f55c14

removed unused variables in spdy test

3049888

made it possible to provide your own spdy protocols, with test

d97ed54

knagaitsev requested review from alexander-akait and hiroppy as code owners March 16, 2019 22:04

knagaitsev changed the title ~~Http2 spdy~~ New spdy option to enable/disable HTTP/2 with HTTPS Mar 16, 2019

hiroppy requested changes Mar 18, 2019

View reviewed changes

hiroppy reviewed Mar 18, 2019

View reviewed changes

alexander-akait requested changes Mar 18, 2019

View reviewed changes

removed spdy option in favor of only http2 option, added test using n…

7884dc8

…ode's http2 module

alexander-akait requested changes Mar 20, 2019

View reviewed changes

knagaitsev added 2 commits March 21, 2019 01:09

deprecation warning to not use https.spdy option, adjust http2 test t…

9167003

…o run only within certain versions

slightly better backwards compatability with https.spdy deprecated op…

21afdb0

…tion

JakobJingleheimer mentioned this pull request Mar 28, 2019

Question: What is the status of HTTP/2 support? #1713

Closed

3 tasks

alexander-akait reviewed Mar 29, 2019

View reviewed changes

knagaitsev added 2 commits March 30, 2019 00:31

default to http2 unless it is explicitly set to false, add more warnings

a79c362

Merge branch 'master' into http2-spdy

76711b8

alexander-akait approved these changes Apr 3, 2019

View reviewed changes

evilebottnawi added 2 commits April 3, 2019 14:44

Merge branch 'master' into http2-spdy

9afb2b9

Merge branch 'master' into http2-spdy

d05a3e5

hiroppy approved these changes Apr 4, 2019

View reviewed changes

alexander-akait merged commit dcd2434 into webpack:master Apr 4, 2019

knagaitsev mentioned this pull request Apr 7, 2019

new Dev Server option: http2 webpack/webpack.js.org#2922

Closed

knagaitsev added the gsoc Google Summer of Code label Aug 13, 2019

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

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

Conversation

knagaitsev commented Mar 16, 2019

For Bugs and Features; did you add new tests?

Motivation / Use-Case

Breaking Changes

Additional Info

codecov bot commented Mar 16, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knagaitsev commented Mar 19, 2019 • edited Loading

alexander-akait left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knagaitsev Mar 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knagaitsev commented Mar 20, 2019

knagaitsev commented Mar 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knagaitsev commented Mar 30, 2019

JakobJingleheimer commented Mar 30, 2019 • edited Loading

alexander-akait left a comment

Choose a reason for hiding this comment

alexander-akait commented Apr 4, 2019

hiroppy left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 16, 2019 •

edited

Loading

knagaitsev commented Mar 19, 2019 •

edited

Loading

knagaitsev Mar 20, 2019 •

edited

Loading

JakobJingleheimer commented Mar 30, 2019 •

edited

Loading