added CSI (\x9b) as additionally allowable escape

[Qix-]

Added \x9b as an additionally allowable ANSI escape code. It's pretty phased out, but can still be used.

The difference between the two is that \x1b requires a following [. Together, both of those characters combined form the CSI. There is the single character alternative, \x9b, that doesn't require the [. It is less common, I speculate, because faulty ANSI implementations will output [37;1m which is more distinct than just 37;1m - don't quote me on that though. All I know is that it has to do with C0 and C1 control codes and a bunch of unicode voodoo.

I doubt many terminal emulators even support it nowadays but it is something you should probably be checking for and should be innocuous to those not using it.

Brought to my attention from chalk/ansi-regex#5 cc @sindresorhus

[Fishrock123]

I think you'd also need to add it on lines such as this? https://github.com/nodejs/node/pull/2521/files#diff-d2acee21faac11dc676c3e4570bad979R998

[Qix-]

You're correct. Will update here in a bit.

[Qix-]

@Fishrock123 done and done.

[silverwind]

cc @rlidwka who has been around these parts recently.

[rlidwka]

I doubt many terminal emulators even support it nowadays

Can you name at least one so we can check it and reference it in the comments?

Also, if you're changing that generator, it'd be nice to add a test.

PS: I have a feeling that this PR adds \x9b[37;1m-like stuff only, where [ is required, which is contrary to what you described. Does the regexp work with \x9b37;1m? Should it?

[Qix-]

Can you name at least one so we can check it and reference it in the comments?

Well it's any ecma-48 compliant terminal. That means it has to support 8-bit sequences. Unfortunately, I can't find a specific list of one.

Also, if you're changing that generator, it'd be nice to add a test.

I can do that 💃

PS: I have a feeling that this PR adds \x9b[37;1m-like stuff only, where [ is required, which is contrary to what you described. Does the regexp work with \x9b37;1m? Should it?

Yes it should, good catch. Not sure why I didn't see that before. I'll make the appropriate edits and whatnot.

[jasnell]

@Qix- ... any updates on this one?

[Qix-]

@jasnell would you be opposed to using ansi-regex? There has been a considerable amount of research and work put into it (check the tests). As it stands, making the current implementation of Node's regex complete would require quite a bit of rewriting.

I know the usage of modules is kind of a pain in a proper node release, but the module is pretty stable and hasn't had to be changed in quite a while. It is simple and could easily be used in a static manner.

[jasnell]

opposed to using it how?

[Qix-]

In lieu of the existing regex strings that I attempted to update in this PR.

[jasnell]

hmm... using the same regex would likely make sense but pulling that module into core might be problematic. Would love to get the thoughts of other @nodejs/ctc folks

[Qix-]

cc @sindresorhus again.

[sindresorhus]

Feel free to take whatever from that module. Anything to make Node.js core better.

[silverwind]

It's a single regex, I think it hardly qualifies as 'pulling a module in', so I'd say go ahead.

[jasnell]

@sindresorhus ... appreciate it!

[jasnell]

@Qix- ... is this still something you'd like to do? If so, would you mind rebasing and updating this to perhaps use the regex from ansi-regex?

[Qix-]

@jasnell is doing that approved? Since it wasn't ever officially stated as "yes, pull in the regex" I didn't bother continuing with anything.

[jasnell]

See #2521 (comment)

[Qix-]

No I know, I mean is it approved by the Node.js team to use it ;) Referring to #2521 (comment).

[jasnell]

As long as @sindresorhus is credited and mention of it is added to our LICENSE file then it should be fine. Basically, just append the contents of https://github.com/chalk/ansi-regex/blob/master/license to https://github.com/nodejs/node/blob/master/LICENSE with a header indicating the source https://github.com/chalk/ansi-regex/. Then add a comment to the source crediting where the regex came from. If others from the @nodejs/ctc have concerns I'm sure they'll bring it up :-)

[Qix-]

Hehe I'm also a collaborator on that repo as well and basically wrote what the regex is currently. IIRC (this is an old pull) Sindre was the one to suggest pulling in the regex since it's better tested anyway.

But yeah I can amend this PR :) Give me a little though!

[jasnell]

:-) ok, make sure to give yourself credit then! :-)