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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix clicking on URLs in the terminal (for VS Code users) #48

Merged
merged 7 commits into from Sep 12, 2017

Conversation

Projects
None yet
2 participants
@johnnyreilly
Collaborator

johnnyreilly commented Sep 8, 2017

EDIT: I misunderstood the original problem as I did this PR. I've left most of what I've written in place but please do read all the way to the end. It will all make a little more sense then I promise 馃槃

The motivation behind this PR has very little to do with getting reported errors closer to ts-loader's "style". (I honestly have no opinion on the style as is.) This PR essentially flows from my addiction to using VS Code as my editor. Let me explain. At present the formatted output of an error looks like this:

ERROR at C:/work/fx-online/src/FxOnline.App/src/containers/deposits/account/GiveNotice/index.tsx(190,44):
no-use-before-declare: variable 'form' used before declaration

Which is great in terms of information. However; there's a gotcha. When using the integrated terminal in VS Code I like being able to click on a file in the terminal and navigate straight there. The existing formatting in the defaultFormatter puts the parens and error location information in with the path itself. And VS Code doesn't interpret it as a link. 馃槩

The change I've made moves the parens onto the next line. This is more like what ts-loader does and also the codeframeFormatter too. The new output looks like this:

ERROR in C:/work/fx-online/src/FxOnline.App/src/containers/deposits/account/GiveNotice/index.tsx
(190,44): no-use-before-declare: variable 'form' used before declaration

EDIT: I misunderstood the reason this wasn't "clickable" - read on and you'll see more.

This is clickable in VS Code 馃憤 I've also changed " at " to " in " as I thought it was a little clearer. If you prefer " at " I'm more than happy to revert that.

My team is running with this fork locally now (yarn linking our way to glory)

What do you think about this? I'd really like to get this merged. More generally I think there could be some work done to allow people to supply their own formatters but in a future PR perhaps.

@johnnyreilly johnnyreilly changed the title from Make errors closer to ts-loader style (allow clicking paths in to Make errors closer to ts-loader style (allow clicking paths in VS Code's integrated terminal) Sep 8, 2017

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Sep 9, 2017

Collaborator

FWIW I realise now that it's possible to supply your own formatter to the plugin (which is awesome). I hadn't grokked that before and so I've updated the documentation to make it a little clearer.

Collaborator

johnnyreilly commented Sep 9, 2017

FWIW I realise now that it's possible to supply your own formatter to the plugin (which is awesome). I hadn't grokked that before and so I've updated the documentation to make it a little clearer.

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Sep 9, 2017

Collaborator

It's just occurred to me that the colouring in the formatters might be what's causing the problem. Let me experiment and report back before you merge this...

Collaborator

johnnyreilly commented Sep 9, 2017

It's just occurred to me that the colouring in the formatters might be what's causing the problem. Let me experiment and report back before you merge this...

@johnnyreilly johnnyreilly changed the title from Make errors closer to ts-loader style (allow clicking paths in VS Code's integrated terminal) to Fix clicking on URLs in the terminal (for VS Code users) Sep 9, 2017

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Sep 9, 2017

Collaborator

Yeah - the actual issue with not being able to click the URLs was the colours. The formatting used to switch colours partway through the message; as it moved from the filename to the error line / column. That's hard to understand; this should clarify it:

ERROR at the/path/index.tsx(190,44):
                            ^ Colour change here!

This colour change screwed up the link clickability which is what got me looking at this in the first place.

Anyway, the fix was simple; just keep the same colour for the filename, line number and column number. This fixes the issue completely; files open when you click on them and to the exact position of the lint / error in question! 馃巻 馃嵕

So I think this is good to merge now - sorry for my original misdiagnosis of the issue.

Collaborator

johnnyreilly commented Sep 9, 2017

Yeah - the actual issue with not being able to click the URLs was the colours. The formatting used to switch colours partway through the message; as it moved from the filename to the error line / column. That's hard to understand; this should clarify it:

ERROR at the/path/index.tsx(190,44):
                            ^ Colour change here!

This colour change screwed up the link clickability which is what got me looking at this in the first place.

Anyway, the fix was simple; just keep the same colour for the filename, line number and column number. This fixes the issue completely; files open when you click on them and to the exact position of the lint / error in question! 馃巻 馃嵕

So I think this is good to merge now - sorry for my original misdiagnosis of the issue.

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Sep 12, 2017

Collaborator

Details of a workaround for those waiting on this PR being merged: https://blog.johnnyreilly.com/2017/09/fork-ts-checker-webpack-plugin-code.html

Collaborator

johnnyreilly commented Sep 12, 2017

Details of a workaround for those waiting on this PR being merged: https://blog.johnnyreilly.com/2017/09/fork-ts-checker-webpack-plugin-code.html

@piotr-oles

This comment has been minimized.

Show comment
Hide comment
@piotr-oles

piotr-oles Sep 12, 2017

Member

Thank you for your work :) Will be released in the v0.2.9

Member

piotr-oles commented Sep 12, 2017

Thank you for your work :) Will be released in the v0.2.9

@piotr-oles piotr-oles merged commit 9b2fa54 into Realytics:master Sep 12, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Sep 12, 2017

Collaborator

Thanks @piotr-oles - I appreciate that!

Collaborator

johnnyreilly commented Sep 12, 2017

Thanks @piotr-oles - I appreciate that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment