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

internals.Server does not allow proto inheritance #2663

Closed
harrytruong opened this issue Jul 26, 2015 · 6 comments
Closed

internals.Server does not allow proto inheritance #2663

harrytruong opened this issue Jul 26, 2015 · 6 comments
Assignees
Labels
Milestone

Comments

@harrytruong
Copy link
Contributor

@harrytruong harrytruong commented Jul 26, 2015

https://github.com/hapijs/hapi/blob/master/lib/server.js#L27

exports = module.exports = internals.Server = function (options) {

    Hoek.assert(this.constructor === internals.Server, 'Server must be instantiated using new');

Is there a reason why this.constructor === internals.Server is used? This breaks prototypical inheritance, preventing the internals.Server class from being extended.

Here's a simplified setup for the issue:

var Server = function(options) {
  if (this.constructor !== Server) throw 'Must use new.'; 
}

// this inheritance helper comes from babel (6to5)
function inherits(subClass, superClass) {
    subClass.prototype = Object.create(
        superClass.prototype,
        { constructor: {
            value: subClass,
            enumerable: false,
            writable: true,
            configurable: true
        } }
    );

    Object.setPrototypeOf(subClass, superClass);
}

// extended child of Server (inheritance style comes from babel/6to5)
var BlueServer = (function (_Server) {
    inherits(BlueServer, _Server);
    function BlueServer(options) {
        Object.getPrototypeOf(BlueServer.prototype).constructor.call(this, options);
    }
    return BlueServer;
})(Server);

The following works properly:

var server = new Server(); // no error, good.
var server = Server(); // throws "use new" error, good.

But this throws an error, when it shouldn't:

var blueServer = new BlueServer(); // throws "use new" error, BAD

Changing (this.constructor !== Server) to !(this instanceof Server) fixes the issue:

var server = new Server(); // no error, good.
var server = Server(); // throws "use new" error, good.

var blueServer = new BlueServer(); // no error, good.
var blueServer = BlueServer(); // throws "use new" error, good.

blueServer instanceof Server; // true, respects prototypical inheritance

This issue is directly related to outmoded/hapi-contrib#43. I wasn't sure if I needed to file the issue in both places.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Jul 26, 2015

Why would you want to inherit from Server?

@harrytruong
Copy link
Contributor Author

@harrytruong harrytruong commented Jul 26, 2015

I have no real reason. I was playing around with reusing Hapi's components in a side project. When I extended from Server to test something, I saw the implementation, and was just curious about the rationale behind it.

@harrytruong
Copy link
Contributor Author

@harrytruong harrytruong commented Jul 26, 2015

I traced the this.constructor === Constructor choice back to @thegoleffect, way back in 2012. Maybe he can explain why it was used instead of this instanceof Constructor?

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Jul 26, 2015

I'm ok changing the style of the implementation. If you want, you can open a PR on this and on the style guide.

@harrytruong
Copy link
Contributor Author

@harrytruong harrytruong commented Jul 27, 2015

PR submitted. Thanks for the consideration/approval. I really appreciate the responsiveness!

@lock
Copy link

@lock lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants