Http2SecureServer Is Not Exported

[shellscape]

• Version: 10.5.0
• Platform: MacOS 10.13.2
• Subsystem: None

While writing tests for a project I found myself wanting to assert that the return value from a function was an instance of Http2SecureServer. However, upon trying to find the export for Http2SecureServer, I wound up at https://github.com/nodejs/node/blob/master/lib/internal/http2/core.js and found that the class isn't actually listed in the exports. So there's no discernible way to run an instanceof against my return variable, as I can't reference the class.

It's an unfortunate limitation; while testing that an object is an instance of tls.Server is fine and good, I would much rather add an extra layer of assertiveness and make sure that my return value was in fact an instance of Http2SecureServer, as the mechanisms in my use case can also return https.Server, which also inherits from tls.Server.

On the surface this seems like a glaring omission. But there's probably a reason for that. What's the scoop?

Update: It would seem the same is true for Http2Server as well.

[ljharb]

What about using .constructor?

[shellscape]

@ljharb I'm not sure how that gets me to

expect(server).toBeInstanceOf(http2.Http2SecureServer);

While server.constructor requires using util.inspect to get anything meaningful in a jest snapshot, and server.constructor.name returns "Http2SecureServer" but isn't the same as asserting the instance. Regardless, this still seems like a rather large omission and unnecessary pain point. I'd like to hear from the folks who worked on this to learn why it was left out of the exports.

[apapirovski]

@shellscape Why not just use http2.createSecureServer().constructor in your tests? Node.js won't export anything other than Http2SecureServer from createSecureServer().

[targos]

This makes me wonder why the API is http2.createSecureServer() and not new http2.SecureServer().

[TimothyGu]

@targos It could easily be:

node/lib/internal/http2/core.js

Lines 2750 to 2762 in 8ab7ea6

	function createSecureServer(options, handler) {
	assertIsObject(options, 'options');
	return new Http2SecureServer(options, handler);
	}
	function createServer(options, handler) {
	if (typeof options === 'function') {
	handler = options;
	options = {};
	}
	assertIsObject(options, 'options');
	return new Http2Server(options, handler);
	}

/cc @jasnell

[ljharb]

Requiring the api to be a constructor more tightly couples the implementation detail that it’s a class.

[TimothyGu]

@ljharb Agreed. In this case however I think it's fair to make Http2Server is a class though.

[apapirovski]

I think this keeps compatibility with how we do things in the other modules. It's easy enough to get the constructor instance for testing that IMO nothing needs to be changed.

[targos]

But in other modules we expose the constructor. http2 is the outlier. It's also inconsistent with itself: http2.Http2ServerRequest and http2.Http2ServerResponse are exposed and the documentation mentions http2.Server and http2.SecureServer which don't exist: https://nodejs.org/dist/latest-v10.x/docs/api/http2.html#http2_class_http2_http2serverrequest

[shellscape]

Yeah the lack of consistency (as classes being exposed through exports are much more common) was why I initially created the issue. As @targos mentions, the way the documentation is written also suggests the class should be available on the export. There doesn't seem to be a cohesiveness between namespaces (net, tls, http, https, http2) as to how classes should or should not be exported. And no hint in the documentation as to which are and which are not.

[apapirovski]

But in other modules we expose the constructor.

And IMO it's a mistake which can lead to all sorts of repercussions later on. The current behaviour also makes it explicit that there's one API to create the server.

It's important to note that the only reason we exposed Http2ServerRequest and Http2ServerResponse is to allow users to create extended versions in an officially supported manner, where we provide a clear contract around their usage. (And by comparison, neither Http2Session nor Http2Stream are exposed.) That choice was also driven by the fact that http exported ServerRequest and ServerResponse. If that was not the case, we would be in a much better position to improve on it going forward.

I also don't see an applicable use case for exposing this particular class as one can easily get access to the constructor in order to do unit testing.

[jasnell]

This design was intentional specifically to allow more flexibility in the implementation as it evolves. Exposing the constrictor directly exposes a number of issues in terms of maintenance. I'm -1 right now for exposing the constructor directly without having a clear use case

[shellscape]

@jasnell @apapirovski I really don't anyone to gloss over the docs being unclear on this. If the constructor is not exposed, I can definitely live with that. But the docs at the very least need help in stating which are exported and which are not.