set isTTY to false if TERM is 'dumb'

[saljam]

This is to stop REPLs/logs from spitting ANSI control codes on terminals which don't support them. (issue #5344)

I'm not sure if this is the best approach (I'm new to node), but it does the job for me.

[Nodejs-Jenkins]

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

Commit saljammaz/node@ef93f3b9f5c1a92544c33ae681e931a605a37c17 has the following error(s):

• Commit message must indicate the subsystem this commit changes

The following commiters were not found in the CLA:

• Salman Aljammaz

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

[TooTallNate]

This seems ok to me, but it should probably go in src/node.js and only affect the main REPL.

[saljam]

Yeah, that was my first approach, but that would mean every program (e.g. npm) would have to implement the same logic.

I'm happy either way though.

[isaacs]

Yeah, I think that this should not club the isTTY setting. I've been on the fence about it, because it's a very elegant way to make programs not behave as if there is a user connected (and thus, if colors should be shown) but I think really, they ought to just sniff isTTY and env.TERM themselves. Bottom line, a dumb terminal is a tty, so isatty(2) returns true, and isTTY should thus be true.

So, yeah.. what @TooTallNate said. In the node repl, default to the "no readline" behavior if NODE_NO_READLINE or TERM==dumb, and send pull reqs to other libs that don't behave properly. It's less elegant, but also less magically lying.

[saljam]

Hello again. @isaacs, @TooTallNate, yeah, that makes total sense. I stuck it in node.js for now. I might get around to doing the same for npm soon.

[chrisdickinson]

Do we want to merge this for v0.12?

[saljam]

Whoa I completely forgot about this. I think I might have deleted this pull request's branch. Is it still mergeable? Do you need me to rebase this patch or anything?

Also, is the Jenkins failing tests message relevant?

[jasnell]

@saljam ... I know it's been a while, but is this still an issue as far as you can tell? And if so, are you interested in updating the PR?

[saljam]

@jasnell seems like node still spits out control codes when TERM=dumb, so yeah I think this is still an issue.

I've since deleted my fork on GitHub, so I'm not sure how to update the commits in this one. Should I issue a new PR? It's just a rebase.

[saljam]

OK abandoned in favour of #25506.