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.

Sign up for GitHub

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

Enable click to go to error in console part 2! #6502

Merged
merged 4 commits into from
Mar 10, 2019
Merged

Enable click to go to error in console part 2! #6502

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c8ea6fd
reattempt changes
johnnyreilly Feb 23, 2019
5b22720
formatter fixes
johnnyreilly Feb 24, 2019
c3b63ba
fix missing colon in path
johnnyreilly Feb 24, 2019
32b1341
revert path tweaking amends as per discussion with @ianschmitz
johnnyreilly Mar 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions packages/react-dev-utils/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ measureFileSizesBeforeBuild(buildFolder).then(previousFileSizes => {
});
```

#### `formatWebpackMessages({errors: Array<string>, warnings: Array<string>}): {errors: Array<string>, warnings: Array<string>}`
#### `formatWebpackMessages({errors: Array<string>, warnings: Array<string>}): {errors: Array<string>, warnings: Array<string>}, filePathToExclude: string)`

Extracts and prettifies warning and error messages from webpack [stats](https://github.com/webpack/docs/wiki/node.js-api#stats) object.

Expand Down Expand Up @@ -332,7 +332,8 @@ Creates a Webpack compiler instance for WebpackDevServer with built-in helpful m
The `args` object accepts a number of properties:

- **appName** `string`: The name that will be printed to the terminal.
- **config** `Object`: The webpack configuration options to be provided to the webpack constructor.
- **appPath** `string`: The path to the application. Used to make error / warning paths pithier.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the typo? Looks good to me? I can confirm that "pithier" is a word 😄 https://en.wiktionary.org/wiki/pithier

- **config** `Object`: The webpack configuration options to be provided to the webpack constructor.
- **devSocket** `Object`: Required if `useTypeScript` is `true`. This object should include `errors` and `warnings` which are functions accepting an array of errors or warnings emitted by the type checking. This is useful when running `fork-ts-checker-webpack-plugin` with `async: true` to report errors that are emitted after the webpack build is complete.
- **urls** `Object`: To provide the `urls` argument, use `prepareUrls()` described below.
- **useYarn** `boolean`: If `true`, yarn instructions will be emitted in the terminal instead of npm.
Expand Down
11 changes: 7 additions & 4 deletions packages/react-dev-utils/WebpackDevServerUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ function printInstructions(appName, urls, useYarn) {

function createCompiler({
appName,
appPath,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should give this a default value of ''?

Copy link
Contributor Author

@johnnyreilly johnnyreilly Feb 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we give it a default value of '' then it somewhat becomes purposeless; appPath is passed around so we can strip the prefix path from lots of places. eg C:/source/my-app/src/something.tsx -> src/something.tsx

I'm beginning to have mixed feelings about the path prefix stripping actually. Whilst it initially seemed like a good idea and reduced "noise" that rather enforces a certain way of working with your codebase. Let me explain:

Consider the following (not unusual) project structure:

package.json
src/
    client/
        src/
            package.json
            // ...
    server/
        src
            package.json
            // ...

With this structure it's quite common to want to open up in the root of that structure and work from there, hacking on both client and server at the same time. You may fire up the VS Code and, run a yarn start:client script which behind the scenes does this cd src/client && yarn start

Without the stripping path prefixes approach this works fine. The paths that appear in the console are clickable as they are complete paths. However with path stripping in effect VS Code doesn't know where to go when you're working in the root of the project. Where is this src/ you speak of? It's confused and not unreasonably.

We lose click to go to error in console entirely in this scenario due to path prefix stripping. You can work around it by opening up only in the root of the client folder and working there. Then it'll be fine. But this feels like we're rather forcing a way of working upon users.

What do you think? My feeling is that users would probably be better served if we dropped the path prefix stripping.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed explanation.

I agree. All of our path stripping solutions so far have had caveats and been somewhat complicated. Maybe the best first implementation is to use full paths. If we can come up with a robust solution in the future, then it will be an improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid - I'll make it so 👍

config,
devSocket,
urls,
Expand Down Expand Up @@ -145,12 +146,13 @@ function createCompiler({
});
});

const formatter = typescriptFormatter(appPath.replace(/\\/g, '/'));
forkTsCheckerWebpackPlugin
.getCompilerHooks(compiler)
.receive.tap('afterTypeScriptCheck', (diagnostics, lints) => {
const allMsgs = [...diagnostics, ...lints];
const format = message =>
`${message.file}\n${typescriptFormatter(message, true)}`;
`${message.file}\n${formatter(message, true)}`;

tsMessagesResolver({
errors: allMsgs.filter(msg => msg.severity === 'error').map(format),
Expand Down Expand Up @@ -209,7 +211,8 @@ function createCompiler({
}
}

const messages = formatWebpackMessages(statsData);
const filePathToExclude = appPath.replace(/\\/g, '/');
const messages = formatWebpackMessages(statsData, filePathToExclude);
const isSuccessful = !messages.errors.length && !messages.warnings.length;
if (isSuccessful) {
console.log(chalk.green('Compiled successfully!'));
Expand All @@ -227,14 +230,14 @@ function createCompiler({
messages.errors.length = 1;
}
console.log(chalk.red('Failed to compile.\n'));
console.log(messages.errors.join('\n\n'));
console.log(messages.errors.join('\n\n').replace('//', ''));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this and the following replace() for? The regex doesn't look right but I am low on coffee.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also related to path prefix stripping - both the console and the Error Overlay are driven from the same information. This removes a double // from the console that would otherwise be printed. Alas the // is necessary to make the Error Overlay display the colon as expected, eg C:/

See my other comment but I'm now of the opinion that path prefix stripping might not be a good idea. I'd be happy to back that out.

return;
}

// Show warnings if no errors were found.
if (messages.warnings.length) {
console.log(chalk.yellow('Compiled with warnings.\n'));
console.log(messages.warnings.join('\n\n'));
console.log(messages.warnings.join('\n\n').replace('//', ''));

// Teach some ESLint tricks.
console.log(
Expand Down
20 changes: 11 additions & 9 deletions packages/react-dev-utils/formatWebpackMessages.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ function isLikelyASyntaxError(message) {

// Cleans up webpack error messages.
// eslint-disable-next-line no-unused-vars
function formatMessage(message, isError) {
function formatMessage(message, filePathToExclude) {
let lines = message.split('\n');

// Strip Webpack-added headers off errors/warnings
Expand Down Expand Up @@ -65,7 +65,9 @@ function formatMessage(message, isError) {
lines.splice(1, 1);
}
// Clean up file name
lines[0] = lines[0].replace(/^(.*) \d+:\d+-\d+$/, '$1');
lines[0] = lines[0]
.replace(/^(.*) \d+:\d+-\d+$/, '$1')
.replace(filePathToExclude, '/');

// Cleans up verbose "module not found" messages for files and packages.
if (lines[1] && lines[1].indexOf('Module not found: ') === 0) {
Expand Down Expand Up @@ -109,13 +111,13 @@ function formatMessage(message, isError) {
return message.trim();
}

function formatWebpackMessages(json) {
const formattedErrors = json.errors.map(function(message) {
return formatMessage(message, true);
});
const formattedWarnings = json.warnings.map(function(message) {
return formatMessage(message, false);
});
function formatWebpackMessages(json, filePathToExclude) {
const formattedErrors = json.errors.map(message =>
formatMessage(message, filePathToExclude)
);
const formattedWarnings = json.warnings.map(message =>
formatMessage(message, filePathToExclude)
);
const result = { errors: formattedErrors, warnings: formattedWarnings };
if (result.errors.some(isLikelyASyntaxError)) {
// If there are any syntax errors, show just them.
Expand Down
93 changes: 49 additions & 44 deletions packages/react-dev-utils/typescriptFormatter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,51 +12,56 @@ const codeFrame = require('@babel/code-frame').codeFrameColumns;
const chalk = require('chalk');
const fs = require('fs');

function formatter(message, useColors) {
const hasGetters = typeof message.getFile === 'function';
const colors = new chalk.constructor({ enabled: useColors });
const messageColor = message.isWarningSeverity() ? colors.yellow : colors.red;

let source;

if (hasGetters) {
source =
message.getFile() &&
fs.existsSync(message.getFile()) &&
fs.readFileSync(message.getFile(), 'utf-8');
} else {
source =
message.file &&
fs.existsSync(message.file) &&
fs.readFileSync(message.file, 'utf-8');
}

let frame = '';

if (source) {
frame = codeFrame(
source,
{ start: { line: message.line, column: message.character } },
{ highlightCode: useColors }
)
.split('\n')
.map(str => ' ' + str)
.join(os.EOL);
}

const severity = hasGetters ? message.getSeverity() : message.severity;
function makeFormatter(appPath) {
const pathToExclude = appPath + '/';
const types = { diagnostic: 'TypeScript', lint: 'TSLint' };

return [
messageColor.bold(`${types[message.type]} ${severity.toLowerCase()}: `) +
(hasGetters ? message.getContent() : message.content) +
' ' +
messageColor.underline(
(message.type === 'lint' ? 'Rule: ' : 'TS') + message.code
),
'',
frame,
].join(os.EOL);
return function formatter(message, useColors) {
const { type, severity, file, line, content, code, character } =
typeof message.getFile === 'function'
? {
type: message.getType(),
severity: message.getSeverity(),
file: message.getFile(),
line: message.getLine(),
content: message.getContent(),
code: message.getCode(),
character: message.getCharacter(),
}
: message;

const colors = new chalk.constructor({ enabled: useColors });
const messageColor = message.isWarningSeverity()
? colors.yellow
: colors.red;
const fileAndNumberColor = colors.bold.cyan;

const source =
file && fs.existsSync(file) && fs.readFileSync(file, 'utf-8');
const frame = source
? codeFrame(
source,
{ start: { line: line, column: character } },
{ highlightCode: useColors }
)
.split('\n')
.map(str => ' ' + str)
.join(os.EOL)
: '';

return [
messageColor.bold(`${types[type]} ${severity.toLowerCase()} in `) +
fileAndNumberColor(
`${file.replace(pathToExclude, '')}(${line},${character})`
) +
messageColor(':'),
content +
' ' +
messageColor.underline((type === 'lint' ? 'Rule: ' : 'TS') + code),
'',
frame,
].join(os.EOL);
};
}

module.exports = formatter;
module.exports = makeFormatter;
22 changes: 14 additions & 8 deletions packages/react-dev-utils/webpackHotDevClient.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,13 @@ function handleWarnings(warnings) {

function printWarnings() {
// Print warnings to the console.
var formatted = formatWebpackMessages({
warnings: warnings,
errors: [],
});
var formatted = formatWebpackMessages(
{
warnings: warnings,
errors: [],
},
process.cwd()
);

if (typeof console !== 'undefined' && typeof console.warn === 'function') {
for (var i = 0; i < formatted.warnings.length; i++) {
Expand Down Expand Up @@ -160,10 +163,13 @@ function handleErrors(errors) {
hasCompileErrors = true;

// "Massage" webpack messages.
var formatted = formatWebpackMessages({
errors: errors,
warnings: [],
});
var formatted = formatWebpackMessages(
{
errors: errors,
warnings: [],
},
process.cwd()
);

// Only show the first error.
ErrorOverlay.reportBuildError(formatted.errors[0]);
Expand Down
4 changes: 3 additions & 1 deletion packages/react-scripts/config/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,9 @@ module.exports = function(webpackEnv) {
watch: paths.appSrc,
silent: true,
// The formatter is invoked directly in WebpackDevServerUtils during development
formatter: isEnvProduction ? typescriptFormatter : undefined,
formatter: isEnvProduction
? typescriptFormatter(paths.appPath.replace(/\\/g, '/'))
: undefined,
}),
].filter(Boolean),
// Some libraries import Node modules but don't use them in the browser.
Expand Down
2 changes: 2 additions & 0 deletions packages/react-scripts/scripts/start.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,11 @@ checkBrowsers(paths.appPath, isInteractive)
errors: errors =>
devServer.sockWrite(devServer.sockets, 'errors', errors),
};
const appPath = paths.appPath.replace(/\\/g, '/');
// Create a webpack compiler that is configured with custom messages.
const compiler = createCompiler({
appName,
appPath,
config,
devSocket,
urls,
Expand Down