From 26d4efb02d17cf7d07df0af87a0e34fb9b8c5f27 Mon Sep 17 00:00:00 2001 From: XadillaX Date: Sun, 11 Jun 2017 02:09:35 +0800 Subject: [PATCH 1/8] https: make opts optional & immutable when create `opts` in `createServer` will be immutable that won't change origional opts value. What's more, it's optional which can make `requestListener` be the first argument. Refs: https://github.com/nodejs/node/issues/13584 --- doc/api/https.md | 2 +- lib/https.js | 6 +++++ test/parallel/test-https-immutable-options.js | 22 +++++++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-https-immutable-options.js diff --git a/doc/api/https.md b/doc/api/https.md index f4000335a00770..f6c56ef8ed7efb 100644 --- a/doc/api/https.md +++ b/doc/api/https.md @@ -46,7 +46,7 @@ added: v8.0.0 See [`http.Server#keepAliveTimeout`][]. -## https.createServer(options[, requestListener]) +## https.createServer([options][, requestListener]) diff --git a/lib/https.js b/lib/https.js index 457327d6bbb729..628b26aeee9347 100644 --- a/lib/https.js +++ b/lib/https.js @@ -34,6 +34,12 @@ const { urlToOptions, searchParamsSymbol } = require('internal/url'); function Server(opts, requestListener) { if (!(this instanceof Server)) return new Server(opts, requestListener); + if (typeof opts === 'function') { + requestListener = opts; + opts = undefined; + } + opts = opts ? util._extend({}, opts) : {}; + if (process.features.tls_npn && !opts.NPNProtocols) { opts.NPNProtocols = ['http/1.1', 'http/1.0']; } diff --git a/test/parallel/test-https-immutable-options.js b/test/parallel/test-https-immutable-options.js new file mode 100644 index 00000000000000..6629f580c88814 --- /dev/null +++ b/test/parallel/test-https-immutable-options.js @@ -0,0 +1,22 @@ +'use strict'; + +const common = require('../common'); + +const assert = require('assert'); +const https = require('https'); +const tls = require('tls'); + +const dftProtocol = {}; +tls.convertNPNProtocols([ 'http/1.1', 'http/1.0' ], dftProtocol); + +const opts = { foo: 'bar' }; +const server1 = https.createServer(opts); + +assert.deepStrictEqual(opts, { foo: 'bar' }); +assert.strictEqual(server1.NPNProtocols.compare(dftProtocol.NPNProtocols), 0); + +const mustNotCall = common.mustNotCall('dummy callback'); +const server2 = https.createServer(mustNotCall); + +assert.strictEqual(server2.NPNProtocols.compare(dftProtocol.NPNProtocols), 0); +assert.strictEqual(mustNotCall, server2._events.request); From 2c7ff7bc13744a95c9f31712b3eb71621e4be85d Mon Sep 17 00:00:00 2001 From: XadillaX Date: Sun, 11 Jun 2017 02:46:02 +0800 Subject: [PATCH 2/8] use Object.assign to instead of util._extend --- lib/https.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/https.js b/lib/https.js index 628b26aeee9347..b0f7e18829a70f 100644 --- a/lib/https.js +++ b/lib/https.js @@ -31,14 +31,14 @@ const inherits = util.inherits; const debug = util.debuglog('https'); const { urlToOptions, searchParamsSymbol } = require('internal/url'); -function Server(opts, requestListener) { +function Server(opts = {}, requestListener) { if (!(this instanceof Server)) return new Server(opts, requestListener); if (typeof opts === 'function') { requestListener = opts; opts = undefined; } - opts = opts ? util._extend({}, opts) : {}; + opts = opts ? Object.assign({}, opts) : {}; if (process.features.tls_npn && !opts.NPNProtocols) { opts.NPNProtocols = ['http/1.1', 'http/1.0']; From b3a1ac6e7f357063c14126a1e4bf1dac76130303 Mon Sep 17 00:00:00 2001 From: XadillaX Date: Sun, 11 Jun 2017 02:48:12 +0800 Subject: [PATCH 3/8] use the default message in common.mustNotCall --- test/parallel/test-https-immutable-options.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-https-immutable-options.js b/test/parallel/test-https-immutable-options.js index 6629f580c88814..6c27c6412cac1a 100644 --- a/test/parallel/test-https-immutable-options.js +++ b/test/parallel/test-https-immutable-options.js @@ -15,7 +15,7 @@ const server1 = https.createServer(opts); assert.deepStrictEqual(opts, { foo: 'bar' }); assert.strictEqual(server1.NPNProtocols.compare(dftProtocol.NPNProtocols), 0); -const mustNotCall = common.mustNotCall('dummy callback'); +const mustNotCall = common.mustNotCall(); const server2 = https.createServer(mustNotCall); assert.strictEqual(server2.NPNProtocols.compare(dftProtocol.NPNProtocols), 0); From 9414765ce5cab6d8799f8c8f524c69802a69303c Mon Sep 17 00:00:00 2001 From: XadillaX Date: Sun, 11 Jun 2017 03:34:27 +0800 Subject: [PATCH 4/8] update after reviewing --- lib/https.js | 2 +- test/parallel/test-https-immutable-options.js | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/lib/https.js b/lib/https.js index b0f7e18829a70f..cc0e331a517a12 100644 --- a/lib/https.js +++ b/lib/https.js @@ -38,7 +38,7 @@ function Server(opts = {}, requestListener) { requestListener = opts; opts = undefined; } - opts = opts ? Object.assign({}, opts) : {}; + opts = util._extend({}, opts); if (process.features.tls_npn && !opts.NPNProtocols) { opts.NPNProtocols = ['http/1.1', 'http/1.0']; diff --git a/test/parallel/test-https-immutable-options.js b/test/parallel/test-https-immutable-options.js index 6c27c6412cac1a..de68eb0d8d028a 100644 --- a/test/parallel/test-https-immutable-options.js +++ b/test/parallel/test-https-immutable-options.js @@ -7,16 +7,20 @@ const https = require('https'); const tls = require('tls'); const dftProtocol = {}; -tls.convertNPNProtocols([ 'http/1.1', 'http/1.0' ], dftProtocol); +tls.convertNPNProtocols([ 'http/1.1' ], dftProtocol); -const opts = { foo: 'bar' }; +const opts = { foo: 'bar', NPNProtocols: [ 'http/1.1' ] }; const server1 = https.createServer(opts); -assert.deepStrictEqual(opts, { foo: 'bar' }); +assert.deepStrictEqual(opts, { foo: 'bar', NPNProtocols: [ 'http/1.1' ] }); assert.strictEqual(server1.NPNProtocols.compare(dftProtocol.NPNProtocols), 0); const mustNotCall = common.mustNotCall(); const server2 = https.createServer(mustNotCall); +// validate that `createServer` can work with no arguments +tls.convertNPNProtocols([ 'http/1.1', 'http/1.0' ], dftProtocol); +assert.ok(server2); assert.strictEqual(server2.NPNProtocols.compare(dftProtocol.NPNProtocols), 0); -assert.strictEqual(mustNotCall, server2._events.request); +assert.strictEqual(server2.listeners('request').length, 1); +assert.strictEqual(server2.listeners('request')[0], mustNotCall); From c798af825c6f8d731b91926f1c65ae6842a2e16e Mon Sep 17 00:00:00 2001 From: XadillaX Date: Sun, 11 Jun 2017 03:54:00 +0800 Subject: [PATCH 5/8] add test case for no argument --- ...options.js => test-https-argument-of-creating.js} | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) rename test/parallel/{test-https-immutable-options.js => test-https-argument-of-creating.js} (74%) diff --git a/test/parallel/test-https-immutable-options.js b/test/parallel/test-https-argument-of-creating.js similarity index 74% rename from test/parallel/test-https-immutable-options.js rename to test/parallel/test-https-argument-of-creating.js index de68eb0d8d028a..8433a00a2a463a 100644 --- a/test/parallel/test-https-immutable-options.js +++ b/test/parallel/test-https-argument-of-creating.js @@ -9,18 +9,28 @@ const tls = require('tls'); const dftProtocol = {}; tls.convertNPNProtocols([ 'http/1.1' ], dftProtocol); +// test for immutable `opts` const opts = { foo: 'bar', NPNProtocols: [ 'http/1.1' ] }; const server1 = https.createServer(opts); assert.deepStrictEqual(opts, { foo: 'bar', NPNProtocols: [ 'http/1.1' ] }); assert.strictEqual(server1.NPNProtocols.compare(dftProtocol.NPNProtocols), 0); + +// validate that `createServer` can work with the only argument requestListener const mustNotCall = common.mustNotCall(); const server2 = https.createServer(mustNotCall); -// validate that `createServer` can work with no arguments tls.convertNPNProtocols([ 'http/1.1', 'http/1.0' ], dftProtocol); assert.ok(server2); assert.strictEqual(server2.NPNProtocols.compare(dftProtocol.NPNProtocols), 0); assert.strictEqual(server2.listeners('request').length, 1); assert.strictEqual(server2.listeners('request')[0], mustNotCall); + + +// validate that `createServer` can work with no arguments +const server3 = https.createServer(); + +assert.ok(server3); +assert.strictEqual(server3.NPNProtocols.compare(dftProtocol.NPNProtocols), 0); +assert.strictEqual(server3.listeners('request').length, 0); From 5de8cf2eb33072fc24bea4a6417e40540e704667 Mon Sep 17 00:00:00 2001 From: XadillaX Date: Sun, 11 Jun 2017 04:10:57 +0800 Subject: [PATCH 6/8] use blocks to instead of server1, server2, ... --- .../test-https-argument-of-creating.js | 36 ++++++++++--------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/test/parallel/test-https-argument-of-creating.js b/test/parallel/test-https-argument-of-creating.js index 8433a00a2a463a..08b647da55b435 100644 --- a/test/parallel/test-https-argument-of-creating.js +++ b/test/parallel/test-https-argument-of-creating.js @@ -7,30 +7,34 @@ const https = require('https'); const tls = require('tls'); const dftProtocol = {}; -tls.convertNPNProtocols([ 'http/1.1' ], dftProtocol); // test for immutable `opts` -const opts = { foo: 'bar', NPNProtocols: [ 'http/1.1' ] }; -const server1 = https.createServer(opts); +{ + const opts = { foo: 'bar', NPNProtocols: [ 'http/1.1' ] }; + const server = https.createServer(opts); -assert.deepStrictEqual(opts, { foo: 'bar', NPNProtocols: [ 'http/1.1' ] }); -assert.strictEqual(server1.NPNProtocols.compare(dftProtocol.NPNProtocols), 0); + tls.convertNPNProtocols([ 'http/1.1' ], dftProtocol); + assert.deepStrictEqual(opts, { foo: 'bar', NPNProtocols: [ 'http/1.1' ] }); + assert.strictEqual(server.NPNProtocols.compare(dftProtocol.NPNProtocols), 0); +} // validate that `createServer` can work with the only argument requestListener -const mustNotCall = common.mustNotCall(); -const server2 = https.createServer(mustNotCall); +{ + const mustNotCall = common.mustNotCall(); + const server = https.createServer(mustNotCall); -tls.convertNPNProtocols([ 'http/1.1', 'http/1.0' ], dftProtocol); -assert.ok(server2); -assert.strictEqual(server2.NPNProtocols.compare(dftProtocol.NPNProtocols), 0); -assert.strictEqual(server2.listeners('request').length, 1); -assert.strictEqual(server2.listeners('request')[0], mustNotCall); + tls.convertNPNProtocols([ 'http/1.1', 'http/1.0' ], dftProtocol); + assert.strictEqual(server.NPNProtocols.compare(dftProtocol.NPNProtocols), 0); + assert.strictEqual(server.listeners('request').length, 1); + assert.strictEqual(server.listeners('request')[0], mustNotCall); +} // validate that `createServer` can work with no arguments -const server3 = https.createServer(); +{ + const server = https.createServer(); -assert.ok(server3); -assert.strictEqual(server3.NPNProtocols.compare(dftProtocol.NPNProtocols), 0); -assert.strictEqual(server3.listeners('request').length, 0); + assert.strictEqual(server.NPNProtocols.compare(dftProtocol.NPNProtocols), 0); + assert.strictEqual(server.listeners('request').length, 0); +} From dc2291cc4fc336377916765e3af033644116235e Mon Sep 17 00:00:00 2001 From: XadillaX Date: Sun, 11 Jun 2017 23:59:21 +0800 Subject: [PATCH 7/8] add test skip --- test/parallel/test-https-argument-of-creating.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/parallel/test-https-argument-of-creating.js b/test/parallel/test-https-argument-of-creating.js index 08b647da55b435..87d934316f887c 100644 --- a/test/parallel/test-https-argument-of-creating.js +++ b/test/parallel/test-https-argument-of-creating.js @@ -1,6 +1,10 @@ 'use strict'; const common = require('../common'); +if (!common.hasCrypto) { + common.skip('missing crypto'); + return; +} const assert = require('assert'); const https = require('https'); From 2232fad8613477d6e8507b751507b88bc18b3cd9 Mon Sep 17 00:00:00 2001 From: XadillaX Date: Tue, 13 Jun 2017 22:45:20 +0800 Subject: [PATCH 8/8] delete default param to prevent some breaking changes --- lib/https.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/https.js b/lib/https.js index cc0e331a517a12..fb220872598315 100644 --- a/lib/https.js +++ b/lib/https.js @@ -31,7 +31,7 @@ const inherits = util.inherits; const debug = util.debuglog('https'); const { urlToOptions, searchParamsSymbol } = require('internal/url'); -function Server(opts = {}, requestListener) { +function Server(opts, requestListener) { if (!(this instanceof Server)) return new Server(opts, requestListener); if (typeof opts === 'function') {