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: refactor construct Interface #4740

Closed
wants to merge 1 commit into from

Conversation

JacksonTian
Copy link
Contributor

Remove the dependency on the arguments.length.

@silverwind silverwind added the readline Issues and PRs related to the built-in readline module. label Jan 18, 2016
@jasnell
Copy link
Member

jasnell commented Jan 18, 2016

@jasnell
Copy link
Member

jasnell commented Jan 18, 2016

I hadn't noticed the Interface constructor before... what an odd way of initializing the object... this change looks good but let's see what CI says :-)

@cjihrig
Copy link
Contributor

cjihrig commented Jan 18, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented Jan 22, 2016

CI failures look unrelated. LGTM

@TooTallNate
Copy link
Contributor

I'm pretty sure it was related to backwards compat with a really old node version (0.6?). Definitely safe to remove at this point.

@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

@MylesBorins
Copy link
Contributor

ci: https://ci.nodejs.org/job/node-test-pull-request/3021/ (rebased against master)

@nodejs/collaborators PTAL

@MylesBorins MylesBorins self-assigned this Jun 17, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Jun 20, 2016

LGTM. Let's try another CI: https://ci.nodejs.org/job/node-test-pull-request/3034/

@MylesBorins
Copy link
Contributor

MylesBorins commented Jul 5, 2016

one more CI. Will land later today if green https://ci.nodejs.org/job/node-test-pull-request/3178/

}

this._sawReturn = false;

EventEmitter.call(this);
var historySize;

if (arguments.length === 1) {
if (input && input.input) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change in behaviour intentional? If input is a custom implementation that happens to have an input property, then it will cause the remaining arguments to be ignored. Can we find a safer way replicating the arguments length check? Maybe the ugly/obvious if (input && !output && !completer && !terminal) approach would be the most appropriate?

Copy link
Contributor

@MylesBorins MylesBorins Jul 5, 2016

Choose a reason for hiding this comment

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

thanks @rmg for scoping that out

thoughts @nodejs/collaborators?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, why not just keep arguments.length check?

Copy link
Contributor

Choose a reason for hiding this comment

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

arguments.length wouldn't work with the way createInterface() was refactored - it now always passes 4 args, even if they're undefined.

Copy link
Member

Choose a reason for hiding this comment

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

How about changing it then to

exports.createInterface = (...args) => new Interface(...args);

?

That should solve the problem and, at the same time, still looks clean and elegant IMO.

Copy link
Member

Choose a reason for hiding this comment

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

Same should be applied for the constructor though, but not hard to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

facepalm
this is targeting master, so I guess that's an option. Would also match lib/console.js, now that I look.

Copy link
Member

@RReverser RReverser Jul 6, 2016

Choose a reason for hiding this comment

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

In fact, given that Interface allows calling itself without new (and unless I'm missing something), it might be even reduced to exports.createInterface = Interface; (in which case I don't really see why two separate methods are exported, unless we want to move Interface to proper ES6 class and effectively break compatibility by forbidding calls without new in future).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no. It doesn't change the behaviour. if (input && input.input) { is used for check first argument is options object.

Remove the dependency on the arguments.length.
@jasnell
Copy link
Member

jasnell commented Jan 6, 2017

@MylesBorins @JacksonTian ... still want this? Looks like it fell through the cracks

@JacksonTian
Copy link
Contributor Author

Yes.

@JacksonTian
Copy link
Contributor Author

landed in dad98bf thanks all.

JacksonTian added a commit that referenced this pull request Jan 13, 2017
Remove the dependency on the arguments.length.

PR-URL: #4740
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
Remove the dependency on the arguments.length.

PR-URL: nodejs#4740
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 23, 2017
Remove the dependency on the arguments.length.

PR-URL: nodejs#4740
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017
Remove the dependency on the arguments.length.

PR-URL: nodejs#4740
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
Remove the dependency on the arguments.length.

PR-URL: nodejs#4740
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
Remove the dependency on the arguments.length.

PR-URL: #4740
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
Remove the dependency on the arguments.length.

PR-URL: #4740
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@MylesBorins
Copy link
Contributor

I've gone ahead and landed this into lts staging. Please let me know if it should be taken out

@MylesBorins MylesBorins added land-on-v4.x and removed lts-watch-v4.x stalled Issues and PRs that are stalled. labels Mar 8, 2017
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
Remove the dependency on the arguments.length.

PR-URL: #4740
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
Remove the dependency on the arguments.length.

PR-URL: #4740
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
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.