Skip to content

TS error in file can no longer be opened at the error location (v4 -> v5) #481

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

Closed
robcrocombe opened this issue Jul 20, 2020 · 9 comments · Fixed by #489
Closed

TS error in file can no longer be opened at the error location (v4 -> v5) #481

robcrocombe opened this issue Jul 20, 2020 · 9 comments · Fixed by #489

Comments

@robcrocombe
Copy link
Contributor

robcrocombe commented Jul 20, 2020

Hi, I'm upgrading from v4 to v5, and noticed the issue location in the code frame display is different. It used to be displayed with brackets around the location, and without the range, e.g: /customer.ts(86,16). But now it has a space and no brackets e.g: /customer.ts 86:16-24.

This is an issue for me as in iTerm I could cmd+click the file name to open it in my editor (Sublime Text), and it would open with the cursor on the line where the error occurred. With v5 however, the new formatting means only the file will open - not the location where the error happened. This is a big productivity issue for me.

Is there any way to restore some of the old behaviour or customise the output file location to display it without range and in brackets? Either in this library or something I can do to my Webpack config. Thanks!

Current behavior

In v5, the location with range is given after a space.

ERROR in /.../customer.ts 86:16-24
TS2551: Property 'giveName' does not exist on type 'Customer'. Did you mean 'givenName'?
    84 |     }
    85 |     if (c.givenName) {
  > 86 |       return c.giveName;
       |                ^^^^^^^^
    87 |     }
    88 |     if (c.familyName) {
    89 |       return c.familyName;

Expected behavior

In v4, the location is given without range, in brackets next to the file.

In order for editors like Sublime and VSCode to open at the location, I think the format needs to be FILE_NAME(LINE,COL) or FILE_NAME:LINE:COL.

ERROR in /.../customer.ts(86,16):
86:16 Property 'giveName' does not exist on type 'Customer'. Did you mean 'givenName'?
    84 |     }
    85 |     if (c.givenName) {
  > 86 |       return c.giveName;
       |                ^
    87 |     }
    88 |     if (c.familyName) {
    89 |       return c.familyName;

Steps to reproduce the issue

Get a TypeScript error.

Issue reproduction repository

None provided.

Environment

  • fork-ts-checker-webpack-plugin: 5.0.7
  • typescript: 3.9.7
  • eslint: n/a
  • webpack: 4.41.6
  • os: MacOS
@piotr-oles
Copy link
Collaborator

Hi!
I understand your point, but we are using standard webpack formatted (or we mimic it in the async mode). Therefore, we don't want to introduce custom behavior to satisfy every use-case.
What I would suggest is tapping into the issues hook and removing issue.location.end information for each issue. If you need more guidance on how to do it, let me know ;)

@robcrocombe
Copy link
Contributor Author

robcrocombe commented Jul 20, 2020

Hi @piotr-oles. Darn, that's a shame. Interesting suggestion, but am I right that will still leave a space between the file name and the location, making it non-linkable?

Kind of surprising that's the Webpack standard, everything I've looked up today about file links seem to use :line:column

@piotr-oles
Copy link
Collaborator

You are right, it wouldn't help you :/ The formatting logic is here. I think this issue should be directed to the webpack's team :)

@johnnyreilly
Copy link
Member

johnnyreilly commented Jul 25, 2020

I think the lack of clickable console errors is unfortunate; with v4 we offered the ability to supply a formatter: (quote from v4 docs)

formatter 'default' | 'codeframe' | ((message: NormalizedMessage, useColors: boolean) => string): Formatter for diagnostics and lints. By default uses default formatter. You can also pass your own formatter as a function (see src/NormalizedMessage.js and src/formatter/ for api reference).

https://github.com/TypeStrong/fork-ts-checker-webpack-plugin/tree/074b2bf036e670ab528216ccd89523161246cdc3#options

This is a way to get around formatting decisions in webpack that aren't console click friendly whilst not breaking standard usage.

Is there any reason we couldn't bring this back? Clickable console errors are a tremendously useful feature! (I love being able to click in VS Code's terminal and bounce straight to the problem - keeps me in flow)

@piotr-oles
Copy link
Collaborator

Currently, in the async: false mode, we don't have much control over the header as it's generated by webpack based on reported errors. So to make it consistent, in the async: true mode we have a separate formatter for the header (WebpackFormatter.ts) and another for the content (this one is configurable - basic or codeframe). So even if we would have a function API, we couldn't change the header.

I was thinking about this a little bit more, and I changed my mind. I think that being consistent with the webpack format shouldn't be more important than developer experience. If the FILE_NAME(LINE,COL) format is better handled by IDE's, it makes sense to use this format. I wouldn't like to have a separate configuration for it - it should work good OOTB. I'm open to PR - change should be simple in the formatIssueLocation, IssueWebpackError and createWebpackFormatter :)

@johnnyreilly
Copy link
Member

I changed my mind. I think that being consistent with the webpack format shouldn't be more important than developer experience

Thanks @piotr-oles - I completely agree!

@robcrocombe fancy having a go at a PR? 🤗

@robcrocombe
Copy link
Contributor Author

@robcrocombe fancy having a go at a PR? 🤗

Lovely! Yes, I should be able to do that. Is there a preference on the format? I think FILE_NAME:LINE:COL might be more universally accepted.

@johnnyreilly
Copy link
Member

johnnyreilly commented Jul 25, 2020

Looking at this reference: microsoft/vscode#66957 (comment)

To quote @Tyriar:

there are several formats supported for line/col but they're all on the same line something like this file:line:col, file(line:col), etc.

I think that file(line:col) was what we previously supported but you may be right that file:line:col is more widely supported. So it could be worth going for that.

piotr-oles pushed a commit that referenced this issue Jul 28, 2020
As discussed in #481, changes the format of TypeScript error file locations to use file:line:col. This means files can be opened from the terminal at the location the error occurred (e.g. cmd+click on Mac).

Closes: #481
@piotr-oles
Copy link
Collaborator

🎉 This issue has been resolved in version 5.0.12 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging a pull request may close this issue.

3 participants