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: parse newer tsc format and strip ansi colors #20

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andrzej-woof
Copy link

This is a basic workaround to address two issues:

  1. tsc output includes colors which makes lines non-matching tscErrorLineRegExp -> sanitize input
  2. tsc output has different format than expected (I'd assume newer version of tsc, I'm currently using 5.2.2) -> extend tscErrorLineRegExp with match for an alternative format

My tsc error output looks as follows:

./some/path/filename.ts:12:3 - error TS4567: Compilation error message.

@Gelio
Copy link
Owner

Gelio commented Nov 15, 2023

Hey, thanks for the contribution and bringing up these issues! I haven't tested loose-ts-check with newer TS versions, and it makes sense to do that.

I will probably review it over the upcoming weekend.

Regarding ANSi colors in the output, I'm surprised tsc started including colors there, since when I was testing it, it only included colors when the output was piped to the terminal. When it was piped to another command, or saved in a file, tsc stripped colors from its output. Are you using some flags/configuration that may force colors to appear in the output?

@andrzej-woof
Copy link
Author

andrzej-woof commented Nov 15, 2023

Are you using some flags/configuration that may force colors to appear in the output?

Ah, you're right, I do have pretty enabled in tsconfig.json (no other related flags that I see)
I still think it might be reasonable to handle such case

@Gelio
Copy link
Owner

Gelio commented Nov 15, 2023

Yup, I agree, the tool should handle that case too. Thanks for checking

@Gelio
Copy link
Owner

Gelio commented Nov 26, 2023

Sorry, it took me a while to get to this PR.

Unfortunately, as I see, the tests are failing. It's not only the formatting differences reported by prettier in CI, but there is a unit test that fails when I run it locally.

As for adding this contribution, let me know if you prefer to give you feedback via PR comments, or take over the PR and incorporate it into the codebase myself, with you still appearing in the list of contributors.

@Gelio
Copy link
Owner

Gelio commented Nov 26, 2023

From my research, it looks like this TS error format you're experiencing is nothing new in TypeScript. TypeScript uses this format when --pretty is used.

Invoking the following on TS 5.3.2:

npm run type-check -- --pretty | pbcopy

(which ran tsc --pretty) yielded me this:

�[96msrc/tsc-errors/parse-tsc-errors.ts�[0m:�[93m9�[0m:�[93m39�[0m - �[91merror�[0m�[90m TS2304: �[0mCannot find name 'tscErrorLineRegExp'.

�[7m9�[0m     const errorLineMatch = line.match(tscErrorLineRegExp);
�[7m �[0m �[91m                                      ~~~~~~~~~~~~~~~~~~�[0m

The output contains both ANSI colors codes and the format is different.

However, when I remove the --pretty flag, TS by default does not use ANSI codes in the output when piping to another process, and it uses the previous error format, which loose-ts-check already supports:

at 19:10:56 ❯ npm run type-check | cat

> loose-ts-check@2.0.0 type-check
> tsc

src/tsc-errors/parse-tsc-errors.ts(9,39): error TS2304: Cannot find name 'tscErrorLineRegExp'.

I got the same results on TS 4.9.4.

This lowers the severity and urgency of this PR. This becomes a "parse TS pretty error output". It's still a nice enhancement, so please answer my last question on how you want to proceed with correcting this PR.

@andrzej-woof
Copy link
Author

@Gelio I think it's best if you take over as I don't have enough time to contribute more or in depth knowledge on the underlying issue - as your investigation shown it seems that's more of an addition rather than bug fix and it's up to you if you want to add pretty output support.
I think it would be nice to have, but as long as everything still works without it it's good from my side - one suggestion is that you add it to readme so that others avoid bumping into same problem for the time being.
Thanks :)

@Gelio
Copy link
Owner

Gelio commented Dec 12, 2023

Sure thing, makes sense to add the information about pretty. I encountered this behavior too when I joined a new project recently 😄 That was good to know that the pretty flag changes so much. I'll update the readme when I have a chance 👍

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