Enable click to go to error in console

[johnnyreilly]

At present when using CRA with TypeScript, you can click on an error in the console and go to the erroring file. This is great!

What would be even better, is if you went directly to the line and column where the error is happening like this:

This PR enables this by augmenting the output in the typescriptFormatter. There is some duplication in the console as webpack already displays the filepath alone.

Certainly for myself, I'd take this duplication to unlock the ability to click and go directly to an error. (This is what the default formatter in fork-ts-checker-webpack-plugin does.)

What do you think?

[Timer]

I thought VS Code supported this without a full path, assuming the path matches the workspace folder opened in VS Code. Does it need to be prefixed with ./?

[johnnyreilly]

Does it need to be prefixed with ./?

Sorry I don't follow you? What's the thing that is prefixed with ./?

I see the same path webpack outputs repeated with (lineNumber,columnNumber) suffixed.

Feel like I've missed your meaning? (I'm on my phone - perhaps GitHub is hiding something from me 😄)

[Timer]

Sorry, I can try to explain better!

I thought VSCode supported "Click to open" with relative paths, i.e. ./src/App.tsx instead of full paths (C:\Work\my-app\src\App.tsx).

I'm all good for adding (lineNumber,columnNumber), I just don't want to add the C:\....

[weswigham]

Yeah, vscode supports relative paths in its terminal. You don't need the leading ./, either, something like tests/cases/compiler/declarationEmitQualifiedAliasTypeArgument.ts works fine.

[johnnyreilly]

Awesome! Only problem: how do I get the workspace location in the context of the typescriptFormatter? If I have that I should be able to pare it back to a relative path.

Can I depend upon process.cwd() or __dirname?

[weswigham]

I think process.cwd()'s a pretty good heuristic, usually. Most of the time scripts get executed in the workspace root, from my experience.

[johnnyreilly]

Thanks - I'll experiment with that and report back

[Timer]

@johnnyreilly import config/paths.js and use appPath, probably will need to pass it to constructor when making formatter.

[johnnyreilly]

cheers @Timer - will have a play

[johnnyreilly]

Hey @Timer,

I've made the suggested change; here's what it looks like with it in place:

[ianschmitz]

Looking good! Perhaps we should strip the leading slash?

[Timer]

We need to somehow morph this path with the (above) highlighted line.

[johnnyreilly]

Looking good! Perhaps we should strip the leading slash?

Yeah could do

We need to somehow morph this path with the (above) highlighted line.

I would love to do that! Alas, unless I've misunderstood things, the (above) highlighted line is not part of the formatter output. It's emitted by webpack I think.

Do you know a way to control that?

[Timer]

We control that output here:
https://github.com/facebook/create-react-app/blob/master/packages/react-dev-utils/formatWebpackMessages.js

We should be able to teach it the necessary trick there. :-)

[johnnyreilly]

Nice, nice!

Will experiment...

[johnnyreilly]

Hmmm... There's a number of call sites.

Should be fine:

https://github.com/facebook/create-react-app/blob/aac14ea5798f991c96e94ff29a7797a823ca7196/packages/react-dev-utils/WebpackDevServerUtils.js#L155

Should be fine, this file already imports ./config/paths:

https://github.com/facebook/create-react-app/blob/aac14ea5798f991c96e94ff29a7797a823ca7196/packages/react-scripts/scripts/build.js#L151
https://github.com/facebook/create-react-app/blob/aac14ea5798f991c96e94ff29a7797a823ca7196/packages/react-scripts/scripts/build.js#L156

But I'm not sure how to make this work nicely:

https://github.com/facebook/create-react-app/blob/aac14ea5798f991c96e94ff29a7797a823ca7196/packages/react-dev-utils/webpackHotDevClient.js#L124
https://github.com/facebook/create-react-app/blob/aac14ea5798f991c96e94ff29a7797a823ca7196/packages/react-dev-utils/webpackHotDevClient.js#L167

Any ideas?

[johnnyreilly]

The issue being the way webpackHotDevClient is referenced in webpack.config.dev.js:

require.resolve('react-dev-utils/webpackHotDevClient'),

https://github.com/facebook/create-react-app/blob/5cdf36024635f58e2f36bd796dc36096c2f57191/packages/react-scripts/config/webpack.config.dev.js#L107

Is there a nice way that appPath be passed to webpackHotDevClient?

[johnnyreilly]

@ianschmitz / @Timer - any thoughts on how I can advance this?

[Timer]

Hey @johnnyreilly, I'm a little tight on time right now so I need to defer to some other collaborators.

I'm very interested in moving this forward though (and will get around to it eventually if someone else doesn't).

[johnnyreilly]

That's cool @Timer - when you're ready then let me know!

[ianschmitz]

Yeah I've been looking at it for a little while - this is a tricky one...

Taking a step back, could we do something as simple as stripping out everything before src/...? I haven't put much thought into if that breaks support somewhere or if eject would still work...

[johnnyreilly]

Taking a step back, could we do something as simple as stripping out everything before src/...? I haven't put much thought into if that breaks support somewhere or if eject would still work...

Sounds possible; like you I've no idea of the answers to the questions.

[alexkuz]

@johnnyreilly As I understand this PR only deals with TypeScript, shouldn't we also use filename(line,col) format in JS projects?

Also it's probably better to use filename:line:col format rather than filename(line,col) - for example, iTerm works fine with the former when I'm clicking on it but ignores parenthesis in the latter.

BTW you might be interested in how they did it in TSLint: palantir/tslint#3491

[johnnyreilly]

As I understand this PR only deals with TypeScript, shouldn't we also use filename(line,col) format in JS projects?

This should cater for both js and ts - files; TypeScript will check both and produce errors for either; this just handles the formatting. It's labelled as TypeScript specific as we only start using TypeScript to identify errors in TypeScript and JavaScript when you add a TypeScript file to the project.

Also it's probably better to use filename:line:col format rather than filename(line,col)

In my experience the filename(line,col) format has more widespread support. I used to have a good link detailing stuff about error formatting - lost it alas. Either way, the cool thing about fork-ts-checker-webpack-plugin is that if you want to supply your own formatter you can!

[alexkuz]

Either way, the cool thing about fork-ts-checker-webpack-plugin is that if you want to supply your own formatter you can!

But I'd need to eject for that, right? I'll better try to convince iTerm maintainers to support filename(line,col) format :)

[alexkuz]

if anyone interested, iTerm2 now supports filename(line,col) format (it's not published yet, as far as I know) - kudos to @gnachman for his awesome work and quick response.

[stale]

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

[johnnyreilly]

I'd quite like to get this in - I'm waiting on advice on how to take this forward.

[stale]

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

[johnnyreilly]

Could someone advise on how we could take this forward please?

[ianschmitz]

I've taken a look at webpackHotDevClient and it doesn't seem like there is an easy way to get appPath into the formatter. I'd be okay with using process.cwd() as a first pass to get this in as it will be a great improvement that should work well for most everyone. Worst case I imagine is if the user is running from some subdirectory for whatever reason, we wouldn't strip off the absolute path leading up to src/, which is not the end of the world.

We can create a separate issue to figure out a better way where we can use appPath.

[johnnyreilly]

Cool - when I get a moment I'll give it a go!

[johnnyreilly]

okay - merging is a world of pain. I regret even attempting this. I'll fork and apply my changes to that

[johnnyreilly]

Closing in favour of #6502