Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix broken link escapes #623

Merged

Conversation

matteodepalo
Copy link
Contributor

@matteodepalo matteodepalo commented Sep 7, 2023

A bug has been introduced with the work done here and it relates to the tokenization of ansi escape characters. We noticed it with ansiEscapes.link and I added a test case in this repo which now fails.

@AlCalzone I don't know enough about your ansi-tokenize project, but could this be fixed there?

Here's a screenshot of the failing test:

Screenshot 2023-09-07 at 11 25 04

You can see from the screenshot that the link escape is not being closed properly so the link "leaks" in all the text after that.

@AlCalzone
Copy link
Contributor

I agree this needs to be fixed in ansi-tokenize

@AlCalzone
Copy link
Contributor

Fixed in v0.1.3: https://github.com/AlCalzone/ansi-tokenize/releases/tag/v0.1.3

(except that your test has extra " around the string)

@matteodepalo
Copy link
Contributor Author

@AlCalzone thank you for fixing this so quickly! I've bumped the version in this PR.

@matteodepalo matteodepalo changed the title Add test that shows a bug with ansi-tokenize Fix bug with anzi-tokenize and links Sep 8, 2023
@matteodepalo
Copy link
Contributor Author

@AlCalzone we also saw a difference in the ansi escapes related to colors when upgrading. For example this component

<Box>
  <Text color="red">hello</Text>
  <Text color="green">world</Text>
</Box>

Before it would produce

'�[31mhello�[39m�[32mworld�[39m'

Now it produces

'�[31mhello�[32mworld�[39m'

There seems to be a missing [39m after the first word which I think it's the code to reset the color? Not sure if this is a difference in the way chalk works, maybe it has been updated? The end result doesn't seem to change much in our outputs.

@AlCalzone
Copy link
Contributor

That's expected. ansi-tokenize uses the least amount of ansi codes necessary for a style change.

@matteodepalo
Copy link
Contributor Author

Got it! Make sense, I'll update our tests.

@vadimdemedes
Copy link
Owner

Thanks @matteodepalo for the PR and thanks @AlCalzone for fixing it so quickly 💛

@vadimdemedes vadimdemedes changed the title Fix bug with anzi-tokenize and links Fix broken link escapes Sep 9, 2023
@vadimdemedes vadimdemedes merged commit adc2c0c into vadimdemedes:master Sep 9, 2023
3 checks passed
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.

3 participants