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

feat: Improved errors and warnings #1758

Merged
merged 3 commits into from
Dec 20, 2022
Merged

feat: Improved errors and warnings #1758

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2d827c9
refactor: Better root error handling
rschristian Dec 16, 2022
03eadd1
refactor: Switch prerender plugin to be async for error gen
rschristian Dec 18, 2022
95c5914
refactor: Simplify & improve compilation messages
rschristian Dec 18, 2022
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
34 changes: 24 additions & 10 deletions packages/cli/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
const envinfo = require('envinfo');
const sade = require('sade');
const notifier = require('update-notifier');
const { green } = require('kleur');
const { error } = require('./util');
const pkg = require('../package.json');

Expand All @@ -23,10 +24,6 @@ const commands = require('./commands');
// installHooks();
notifier({ pkg }).notify();

process.on('unhandledRejection', err => {
error(err.stack || err.message);
});

const prog = sade('preact').version(pkg.version);

prog
Expand Down Expand Up @@ -57,7 +54,7 @@ prog
.option('--inlineCss', 'Adds critical CSS to the prerendered HTML', true)
.option('-c, --config', 'Path to custom CLI config', 'preact.config.js')
.option('-v, --verbose', 'Verbose output', false)
.action(commands.build);
.action((src, argv) => exec(commands.build(src, argv)));

prog
.command('watch [src]')
Expand Down Expand Up @@ -85,13 +82,13 @@ prog
.option('-c, --config', 'Path to custom CLI config', 'preact.config.js')
.option('-H, --host', 'Set server hostname', '0.0.0.0')
.option('-p, --port', 'Set server port (default 8080)')
.action(commands.watch);
.action((src, argv) => exec(commands.watch(src, argv)));

prog
.command('info')
.describe('Print out debugging information about the local environment')
.action(() => {
envinfo
.action(() =>
exec(envinfo
.run({
System: ['OS', 'CPU'],
Binaries: ['Node', 'Yarn', 'npm'],
Expand All @@ -104,8 +101,8 @@ prog
],
npmGlobalPackages: ['preact-cli'],
})
.then(info => process.stdout.write(`\nEnvironment Info:${info}\n`));
});
.then(info => process.stdout.write(`\nEnvironment Info:${info}\n`))
));

prog.parse(process.argv, {
alias: {
Expand All @@ -118,3 +115,20 @@ prog.parse(process.argv, {
);
},
});

function exec(cmd) {
cmd.catch(catchExceptions);
}

/**
* @param {Error | import('webpack').StatsError} err
*/
async function catchExceptions(err) {
// Webpack Stats Error
if ('moduleName' in err && 'loc' in err) {
error(`${err.moduleName} ${green(err.loc)}\n${err.message}\n\n`);
}
error(err.stack || err.message || err);
}

process.on('unhandledRejection', catchExceptions);
4 changes: 2 additions & 2 deletions packages/cli/src/lib/webpack/prerender.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const stackTrace = require('stack-trace');
const URL = require('url');
const { SourceMapConsumer } = require('source-map');

module.exports = function (env, params) {
module.exports = async function (env, params) {
params = params || {};

let entry = resolve(env.dest, './ssr-build/ssr-bundle.js');
Expand Down Expand Up @@ -37,7 +37,7 @@ module.exports = function (env, params) {
throw err;
}

handlePrerenderError(err, env, stack, entry);
await handlePrerenderError(err, env, stack, entry);
Copy link
Member Author

Choose a reason for hiding this comment

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

The end result of a prerender error is a good ol' process.exit(1), so we want to block Webpack if we're at this point; there's no point in allowing it to continue whilst we track down the error position.

}
};

Expand Down
6 changes: 4 additions & 2 deletions packages/cli/src/lib/webpack/render-html-plugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ module.exports = async function renderHTMLPlugin(config, env) {
title,
filename,
template: `!!${require.resolve('ejs-loader')}?esModule=false!${template}`,
templateParameters: (compilation, assets, assetTags, options) => {
templateParameters: async (compilation, assets, assetTags, options) => {
let entrypoints = {};
compilation.entrypoints.forEach((entrypoint, name) => {
let entryFiles = entrypoint.getFiles();
Expand All @@ -96,7 +96,9 @@ module.exports = async function renderHTMLPlugin(config, env) {
env,
preRenderData: values,
CLI_DATA: { preRenderData: { url, ...routeData } },
ssr: config.prerender ? prerender({ cwd, dest, src }, values) : '',
ssr: config.prerender
? await prerender({ cwd, dest, src }, values)
: '',
entrypoints,
},
htmlWebpackPlugin: {
Expand Down
107 changes: 42 additions & 65 deletions packages/cli/src/lib/webpack/run-webpack.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@ const ip = require('ip');
const webpack = require('webpack');
const { resolve } = require('path');
const clear = require('console-clear');
const { bold, red, green, magenta } = require('kleur');
const { bold, green, magenta } = require('kleur');
const DevServer = require('webpack-dev-server');
const clientConfig = require('./webpack-client-config');
const serverConfig = require('./webpack-server-config');
const transformConfig = require('./transform-config');
const { error, isDir, warn } = require('../../util');
const { isDir, warn } = require('../../util');

/**
* @param {import('../../../types').Env} env
Expand All @@ -33,16 +33,12 @@ async function devBuild(config, env) {
let serverAddr = `${protocol}://${host}:${bold(config.port)}`;
let localIpAddr = `${protocol}://${ip.address()}:${bold(config.port)}`;

if (stats.hasErrors()) {
process.stdout.write(red('Build failed!\n\n'));
} else {
if (!stats.hasErrors()) {
process.stdout.write(green('Compiled successfully!\n\n'));
process.stdout.write('You can view the application in browser.\n\n');
process.stdout.write(`${bold('Local:')} ${serverAddr}\n`);
process.stdout.write(`${bold('On Your Network:')} ${localIpAddr}\n`);
}

showStats(stats, false);
});

compiler.hooks.failed.tap('CliDevPlugin', rej);
Expand All @@ -69,100 +65,81 @@ async function prodBuild(config, env) {
const clientWebpackConfig = await clientConfig(config, env);
await transformConfig(clientWebpackConfig, config, env);
const clientCompiler = webpack(clientWebpackConfig);
await runCompiler(clientCompiler);

try {
let stats = await runCompiler(clientCompiler);

// Timeout for plugins that work on `after-emit` event of webpack
await new Promise(r => setTimeout(r, 20));

return showStats(stats, true);
} catch (err) {
// eslint-disable-next-line
console.log(err);
}
// Timeout for plugins that work on `after-emit` event of webpack
await new Promise(r => setTimeout(r, 20));
}

/**
* @param {import('webpack').Compiler} compiler
*/
function runCompiler(compiler) {
return new Promise(res => {
return new Promise((res, rej) => {
compiler.run((err, stats) => {
if (err) {
error(err, 1);
}
if (err) rej(err);

showStats(stats, true);
showCompilationIssues(stats);

res(stats);
compiler.close(closeErr => {
if (closeErr) rej(closeErr);
res();
});
});
});
}

function showStats(stats, isProd) {
/**
* @param {import('webpack').Stats} stats
*/
function showCompilationIssues(stats) {
if (stats) {
if (stats.hasErrors()) {
allFields(stats, 'errors')
.map(stripLoaderPrefix)
.forEach(({ message }) => error(message, isProd ? 1 : 0));
if (stats.hasWarnings()) {
allFields(stats, 'warnings').forEach(({ message }) => warn(message));
}

if (stats.hasWarnings()) {
allFields(stats, 'warnings')
.map(stripLoaderPrefix)
.forEach(({ message }) => warn(message));
if (stats.hasErrors()) {
allFields(stats, 'errors').forEach(err => {
throw err;
});
Comment on lines +97 to +104
Copy link
Member Author

Choose a reason for hiding this comment

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

I switched the order of warning & error reporting here; as we bail on errors, a user would need an error-less build in order to be alerted of any warnings which I think is less than ideal.

If they're alerted to warnings first, they can fix those alongside whatever error they're experiencing.

}
}

return stats;
}

/**
* Recursively retrieve all errors or warnings from compilation
*
* @param {import('webpack').Stats} stats
* @param {'warnings' | 'errors'} field
* @returns {import('webpack').StatsError[]}
*/
function allFields(stats, field, fields = [], name = null) {
const info = stats.toJson({
errors: true,
warnings: true,
errorDetails: false,
});

const addCompilerPrefix = msg =>
name ? bold(magenta(name + ': ')) + msg : msg;
if (field === 'errors' && stats.hasErrors()) {

if (field === 'errors') {
fields = fields.concat(info.errors.map(addCompilerPrefix));
}
if (field === 'warnings' && stats.hasWarnings()) {
} else {
fields = fields.concat(info.warnings.map(addCompilerPrefix));
}

if (stats.compilation.children) {
stats.compilation.children.forEach((child, index) => {
const name = child.name || `Child Compiler ${index + 1}`;
const stats = child.getStats();
fields = allFields(stats, field, fields, name);
if (field === 'errors' ? stats.hasErrors() : stats.hasWarnings()) {
fields = allFields(stats, field, fields, name);
}
});
}
return fields;
}

/** Removes all loaders from any resource identifiers found in a string */
function stripLoaderPrefix(str) {
if (typeof str === 'string') {
str = str.replace(
/(?:(\()|(^|\b|@))(\.\/~|\.{0,2}\/(?:[^\s]+\/)?node_modules)\/\w+-loader(\/[^?!]+)?(\?\?[\w_.-]+|\?({[\s\S]*?})?)?!/g,
'$1'
);
str = str.replace(/(\.?\.?(?:\/[^/ ]+)+)\s+\(\1\)/g, '$1');
str = replaceAll(str, process.cwd(), '.');
return str;
}
return str;
}

// https://gist.github.com/developit/1a40a6fee65361d1182aaa22ab8c334c
function replaceAll(str, find, replace) {
let s = '',
index,
next;
while (~(next = str.indexOf(find, index))) {
s += str.substring(index, next) + replace;
index = next + find.length;
}
return s + str.substring(index);
return fields;
}

/**
Expand Down