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

readline: fix calling constructor without new #1385

Closed
wants to merge 1 commit into from

Conversation

rlidwka
Copy link
Contributor

@rlidwka rlidwka commented Apr 9, 2015

This works:

$ iojs -e 'new (require("readline").Interface)({ input: process.stdin, output: process.stdout })'

This doesn't:

$ iojs -e 'require("readline").Interface({ input: process.stdin, output: process.stdout })'
readline.js:98
    input.on('data', ondata);
          ^
TypeError: undefined is not a function
    at new Interface (readline.js:98:11)
    at Object.Interface (readline.js:29:12)
    at [eval]:1:21
    at Object.exports.runInThisContext (vm.js:54:17)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:446:26)
    at evalScript (node.js:413:25)
    at startup (node.js:72:7)
    at node.js:799:3

The reason is that option object detection here relies on a number of arguments passed to the constructor.

But instanceof guard here always makes it four arguments.


So the issue happens when usual instanceof check is combined with logic based on arguments.length.

There are two places in io.js core when it matters. Second one is in buffer.js and it's already handled correctly.

@silverwind silverwind added the readline Issues and PRs related to the built-in readline module. label Apr 9, 2015
@@ -26,7 +26,10 @@ exports.createInterface = function(input, output, completer, terminal) {

function Interface(input, output, completer, terminal) {
if (!(this instanceof Interface)) {
return new Interface(input, output, completer, terminal);
// call the constructor preserving original number of arguments
var self = Object.create(Interface.prototype);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: use const to keep it lexically scoped.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to const in the next commit

@bnoordhuis
Copy link
Member

LGTM with a suggestion.

Previously, we detected options object based on amount of arguments
supplied. But if we're calling readline without new operator,
constructor gets re-called and will always have 4 arguments.
bnoordhuis pushed a commit that referenced this pull request Apr 10, 2015
Previously, we detected options object based on amount of arguments
supplied. But if we're calling readline without new operator,
constructor gets re-called and will always have 4 arguments.

PR-URL: #1385
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@bnoordhuis
Copy link
Member

Cheers @rlidwka, landed in f0bf6bb.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
readline Issues and PRs related to the built-in readline module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants