refactor: use process.stdout.columns instead of dep

[maxrimue]

I was able to remove the dependency window-size and instead use process.stdout.columns which effectively does the same thing but seems to be much less resource hungry.

[addaleax]

The last term in this conjunction seems a bit odd? (Also: typeof process === 'object' is probably the better first check here, at least if there’s a chance this file will ever switch to strict mode?)

[bcoe]

I agree, this should probably look something more like this:

if (typeof process === 'object' && process.stdout && process.stdout.columns) {
  return Math.min(80, process.stdout.colums)
} else {
  return 80
}

I think it's probably more correct to explicitly return 80 than null, this might be a historical bug.

[maxrimue]

Huh, not sure what I was thinking there. What I am wondering now though, why is the output's width limited to 80 columns like that? Since Math.min() always returns the tiniest value of the given parameters, it can only return numbers in the range of <=80 like that.

[bcoe]

we'll need to figure out how to test this new logic to keep coverage at 100%.

You might be able to do something like mock process.stdout.columns in a test for Travis.

[bcoe]

@maxrimue @addaleax this is what I was thinking:

0f0c264

[maxrimue]

@bcoe Yeah, was just about to push something similar. I approve!

[bcoe]

@addaleax @maxrimue this is slated to go out in 6.6.0, and is staged on the next tag; I would love the extra help testing since 6.6.0 is looking like a pretty big release:

npm cache clear; npm i yargs@next

[jonschlinkert]

this is a great example of where packages like window-size would hugely benefit from a pr and discussion so that other packages that need the same functionality don't need to go through this same discovery process and reinvent the wheel. Or worse, never get the improvements at all. Just my 2c.

edit: @maxrimue can you clarify where the "resource intensiveness" was so we can look into in window-size? Clearly that's not good if something is buggy, I'd like to get it figured out.

[maxrimue]

@jonschlinkert I don't right now have the actual data at hand, sorry. However, I can tell you that the resource intensity was relative, window-size was just one of the longer taking parts of the arg parsing in yargs, which lead to us lazy loading it.

The main reason for me to do this PR however is that process.stdout.columns seemingly achieves the same thing that window-size does, without any drawbacks. Generally, I'm a friend of requiring lots of modules, but in this case, requiring and running this module can be replaced by a single object access.

[jonschlinkert]

achieves the same thing that window-size does, without any drawbacks

Your code won't work on Windows 10 or newer, and it will fail if not a TTY. Not sure if those count as drawbacks.

requiring and running this module can be replaced by a single object access.

Great idea. Reminds me of process.argv.slice(2)

[addaleax]

Your code won't work on Windows 10 or newer

Got any references for that? Is that a Node bug?

it will fail if not a TTY

Maybe I’m being naïve, but how does the concept of a window size apply to non-TTYs?

[maxrimue]

@jonschlinkert @addaleax

Your code won't work on Windows 10 or newer

This pr was in fact created on my Windows 10 machine, for me it does work

[jonschlinkert]

Got any references for that? Is that a Node bug?

jonschlinkert/window-size#6. When this pr was made, I verified that there was a bug. As I recall there was a regression in node 7.1.0 on windows, perhaps that was the culprit. Any thoughts @addaleax? (but shouldn't this question have been asked of @maxrimue)?

Maybe I’m being naïve, but how does the concept of a window size apply to non-TTYs?

No, you're right. I meant "is" TTY not non-TTY. e.g. Meaning isn't tty.getWindowSize() still necessary in some cases? I vaguely recall reading something on node's docs that mentioned no longer using tty.getWindowSize() as of 0.6.0, but if that's the case it seems a bit early to completely drop support with a lib as popular as this.

Which circles us back around to my initial point. We use yargs and yargs-parser in something like 15-25 libs. I've seen issues and edge cases on window-size and during research I've done per those discussions. Because of that, these changes seem naive to me. But if this is indeed all that's needed in window-size, it would have been awesome to have that discussion so that we could improve window-size for all users. That's all, nothing more. I don't care about the dependency.

@maxrimue, @addaleax are you saying there is zero chance of issues/edge cases with these code changes? If so, I'll be a happy camper.

[addaleax]

As I recall there was a regression in node 7.1.0 on windows, perhaps that was the culprit.

Yeah, there’s something in the back of my head about that too. Anything related to that should be fixed by now, though.

Meaning isn't tty.getWindowSize() still necessary in some cases?

Huh, I didn’t know that thing existed (also, Node 0.6 was a long time ago…). in any case, yargs’ .travis.yml only contains Node versions >= 0.10, so I would guess older versions don’t count as officially supported?

are you saying there is zero chance of issues/edge cases with these code changes

At the very least one difference to window-size is that this code here doesn’t check process.stderr, which is something you’d probably want in a generic window-size getter. That seems correct here, though, because yargs itself uses only process.stdout (afaik).

[bcoe]

@jonschlinkert I'm a big advocate of the tiny module approach to software development. I've been trying to hit a balance with yargs where we outsource appropriate components, while being mindful of complaints I've been seeing over the past few months along performance and dependency count related lines:

https://twitter.com/guillaumeQD/status/807917750425440256
#521

When @maxrimue began advocating this change, I was hesitant because I knew that window-size was a mature library and had grown to address many edge-cases around window size detection ... I looped in @addaleax who's part of Node's core technical committee, in the hopes to better understand what platforms are under-supported by process.stdout.columns; it was my understanding that Windows was the major caveat as well, however Anna assured me that these issues had been somewhat addressed on newer versions of Node:

@jonschlinkert I apologize for not looping you into this conversation at the outset, this was an oversight. I'd like to better understand what edge-cases we're dropping on the floor by excluding this dependency; this brings up a couple interesting options:

1. pull the dependency back in, but load it conditionally based on OS.
2. heck, could we work with @addaleax and the other core Node folks to better communicate some of the shortcomings around process.stdout.columns, and work on getting these addressed in Node core?

[jonschlinkert]

Node 0.6

Good grief lol. What's funny is that I wrote the correct thing, 0.6.0, but I swear that in my mind I read that as 6.0. Not just today when I wrote it. I mean, since last week sometime when I first read that deprecation info. I probably would have kept internalizing it as 6.0 if @addaleax hadn't said "a long time ago". Which caused me to go, "that wasn't long ago at all, what is she....? ohhhh".. Bottom line => I am my own worst enemy. Lol, try not to hold this against me, I promise I'm only a moron like 20-40% of the time.

@bcoe no need to apologize, I really appreciate the detail.

To be clear, I'm much more interested in just having a solution that consistently works (and won't create issues and regressions), I'm not at all concerned with window-size being a dependency. I'm a big fan of just making the right decision for your code base, and I'm sorry that feel you had to apologize to me about that. That's my fault for how I approached the issue.

Circling back to my initial concerns, I think this was unusual for me in the sense that I just happened to be working on window-size this past week, and when I saw the changes here, the only thing that went through my mind was, "really? Is that all we need to be doing on window-size now?" and if so I feel like a jackass for overcomplicating the code so much - especially now in light of my "rounding error". still, admittedly I don't have as much time to stay current on changes in node.js these days, and I can't help but think based on window-size issues and past research that there are edge cases lurking. But... that doesn't mean there will be edge cases or issues for yargs that way that it's being used here.

I remember it being somewhat of a pain to research when I first created it. But that was a while ago, and a lot has changed. This is basically where my mind was: I'd love to simplify the code for everyone using window-size if we're confident that there won't be edge cases where the other fallbacks are needed (namely .getWindowSize).