Skip to content

Conversation

Qix-
Copy link
Contributor

@Qix- Qix- commented May 10, 2017

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
Copy link
Member

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

@Qix-
Copy link
Contributor Author

Qix- commented May 11, 2017

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
Copy link
Member

Forgot that we removed strip-ansi from Chalk.

@Qix-
Copy link
Contributor Author

Qix- commented May 11, 2017

💃

@sindresorhus
Copy link
Member

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

@Qix-
Copy link
Contributor Author

Qix- commented May 16, 2017

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
Copy link
Member

sindresorhus commented May 16, 2017

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-
Copy link
Contributor Author

Qix- commented May 16, 2017

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
Copy link
Member

Yeah I'll change it.

@Qix- ping :)

@Qix-
Copy link
Contributor Author

Qix- commented Jun 20, 2017

@sindresorhus done and done.

@Qix-
Copy link
Contributor Author

Qix- commented Jun 20, 2017

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

@sindresorhus sindresorhus merged commit 69bebf6 into master Jun 20, 2017
@sindresorhus sindresorhus deleted the urxvt branch June 20, 2017 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants