chalk / ansi-regex

Regular expression for matching ANSI escape codes
MIT License
182 stars 78 forks source link

Support urxvt escapes #13

Closed Qix- closed 7 years ago

Qix- commented 7 years ago

Fixes chalk/strip-ansi#10.

As I mentioned over there these are annoyingly different escape codes.

This PR is a massive performance hit since it has to do backtracking on almost every match. There's no other way to support it, though.

sindresorhus commented 7 years ago

Can you run the Chalk benchmarks before and after this change to ensure the performance degradation is not too large?

Qix- commented 7 years ago

I'm not sure what you mean; chalk shouldn't be affected by this as Chalk doesn't use ansi-regex or strip-ansi.

sindresorhus commented 7 years ago

Forgot that we removed strip-ansi from Chalk.

Qix- commented 7 years ago

💃

sindresorhus commented 7 years ago

This makes the regex really complicated. Can we make it a separate regex and then combine them programmatically instead?

Qix- commented 7 years ago

I feel like the programmatic complexity would far outweigh the regex complexity. This has the added benefit of V8 loading in the entire regex at once while parsing, which will speed up initialization.

sindresorhus commented 7 years ago

I feel like the programmatic complexity would far outweigh the regex complexity.

I'm not sure we're on the same page. This is what I was thinking:

const ansiRegex = /[\u001b\u009b][[()#;?]*(?:[0-9]{1,4}(?:;[0-9]{0,4})*)?[0-9A-PRZcf-nqry=><]/g;
const urxvtRegex = ...;
return new RegExp(ansiRegex.source + '|' + urxvtRegex.source, 'g'), 

This has the added benefit of V8 loading in the entire regex at once while parsing, which will speed up initialization.

That is IMHO a meaningless micro-optimization.

Qix- commented 7 years ago

I knew what you meant but seeing it in code makes it look better than it did in my head.

Yeah I'll change it.

sindresorhus commented 7 years ago

Yeah I'll change it.

@Qix- ping :)

Qix- commented 7 years ago

@sindresorhus done and done.

Qix- commented 7 years ago

Nvm, now it's done and done. Rebased.