I don't think we have to change this in the documentation, since tls.Server() will still require a configuration (a certificate and key at minimum). The benefit of guarding early before tls.Server() is called is to avoid strange JavaScript runtime errors. Huh... I take that back, it doesn't seem to enforce a minimum configuration...

But a test in either way should be added.

@refack the test is in test/parallel/test-https-immutable-options.js, which tested in server2.

Need a test like

// validate that `createServer` can work with no arguments
const server2 = https.createServer();
assert.ok(server2);
assert.strictEqual(server1.NPNProtocols.compare(dftProtocol.NPNProtocols), 0);

@refack done.

I think you can use Object.assign (and give opts a default in L34)

@refack Did you benchmark that util._extend is faster than Object.assign by now? Last I heard it was not, which is why we still use util._extend for most internals.

@addaleax I was about to say that but I'm not sure it matters in the case of starting a server. *shrug*

util._extend() does have significantly more usage in core though according to a quick grep.

Hmmmm, so I'm waiting for a result.

IMHO this is a "cold spot" so I opt for standard 🤷‍♂️
Any way this could be simply

opts = util._extend({}, opts);

@XadillaX I think you're free to decide

> console.time('assign');for(var i=0; i < 1E7; ++i) { Object.assign({1:1, 2:2, 3:3}, {a:1, b:2, c:3}); };console.timeEnd('assign')
assign: 9825.217ms
undefined
> console.time('_extend');for(var i=0; i < 1E7; ++i) { u._extend({1:1, 2:2, 3:3}, {a:1, b:2, c:3}); };console.timeEnd('_extend')
_extend: 6174.819ms
undefined

so util._extend is ~ 60% of Object.assign.
IMHO if it's not better than 50%, Object.assign should be prefered

If util._extend uses more than Object.assign, I think I can change it back to util._extend.

@refack, And if necessary, I think we can open a new PR about changing all util._extend to Object.assign in this file next time. Because it not only contains one util._extend here.

@refack, And if necessary, I think we can open a new PR about changing all util._extend to Object.assign in this file next time. Because it not only contains one util._extend here.

We can do this when V8 optimizes Object.assign to be <= util._extend...

But a test in either way should be added.

It might be best to just omit the argument here, since if the function is called for some odd reason the default message should be slightly more descriptive and good enough.

I think it should suffice to just compare the NPNProtocols references. That way we know for sure that our options weren't overridden:

assert.strictEqual(server1.NPNProtocols, dftProtocol.NPNProtocols);

@mscdex I think we can't, because NPNProtocols hasn't been cached yet. 2 calls to tls.convertNPNProtocols won't return the same reference.

Ah ok I see it now. Perhaps we should at least choose a different set of protocols then, so as to differentiate from the default NPN protocols (perhaps just use one protocol?).

Need a test like

// validate that `createServer` can work with no arguments
const server2 = https.createServer();
assert.ok(server2);
assert.strictEqual(server1.NPNProtocols.compare(dftProtocol.NPNProtocols), 0);

I think you can change this to

opts = Object.assign({}, opts);

Can we replace this with

assert.strictEqual(server2.listeners('request'), 1);
assert.strictEqual(server2.listeners('request')[0], mustNotCall);

The comment below doesn't match the behavior of this line. If we're testing no arguments, it should just be https.createServer().

Otherwise we could add a separate test below for no arguments.

Nit: It doesn't harm but I think this is redundant. An error would be thrown below if server2 is falsy.

@refack, Shall I?

It's a style we now use more often (helps frame every test case), but IMHO it's up to you... that was for the other comment
I'm 👍 for keeping it, but maybe put if just below the const server2 =

Tiny nit: instead of using server1, server2, and server3 we can use block scope.

{
  // test for immutable `opts`
}

{
  // validate that `createServer` can work with the only argument requestListener
}

It's a style we now use more often (helps frame every test case), but IMHO it's up to you...

Can you add this:

if (!common.hasCrypto) {
  common.skip('missing crypto');
  return;
}

Sure. 👌

Minor nit: The optional value is an object, but here reset to undefined, this does not appear consistent :)

@yorkie, Because I think util._extend({}, undefined) performance is better than util._extend({}, {})

Then I think you would make the opts default be unset as well for performance. BTW perf is not that crucial here than consistence and readability, this constructor needn't be called too much times in any of applications :)

The problem is opts = util._extend({}, opts); shows it's not a consistent.

Do you mean that?

...

const realOpts = util._extend({}, opts);

The mean problem is the default value, removing that like https://github.com/nodejs/node/pull/13599/files#r121620077 works for me :)

Note that using default values is potentially a breaking change given that it changes the value of Server.length

Shall I delete the default value?