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.

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

feat: Less verbose error output #206

Merged
merged 2 commits into from
Jul 10, 2017
Merged

feat: Less verbose error output #206

merged 2 commits into from
Jul 10, 2017

Conversation

rkostrzewski
Copy link
Collaborator

+1000 to DX

preact build before:
build-old

preact build after:
build-new

preact watch before:
watch-old

preact watch after:
watch-new

Changes:

  • stats.toJSON() accepts options which could be 'errors-only' (like in webpack.config.js)
  • rewrote babel-loader query stripping (placed by async-component-loader) to be more precise & maintanable (= renamed it 馃槅 )

process.stdout.write(chalk.red('\Build failed!\n\n'));
} else {
process.stdout.write(chalk.green('\nCompiled successfully!\n\n'));
}
Copy link
Member

@reznord reznord Jul 8, 2017

Choose a reason for hiding this comment

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

Better to expose the IP addresses & port message only when the build is successful !!

    if (userPort !== port) {
       process.stdout.write(`Port ${chalk.bold(userPort)} is in use, using ${chalk.bold(port)} instead\n\n`);
    }
    process.stdout.write('You can view the application in browser.\n\n');process.stdout.write(`${chalk.bold('Local:')}            ${serverAddr}\n`);
    process.stdout.write(`${chalk.bold('On Your Network:')}  ${localIpAddr}\n`);

otherwise LGTM !!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That could be possible but I don't know really. webpack-dev-server shows address always on first compilation.

We kinda don't inform users after they fixed the errors if anything occurred. Maybe the output should look like this:

You can view the application in browser.

   Local: ...
   On Your Network: ...

Build failed!
--Some error message--
Compiled successfully --date stamp--
Compiled successfully --date stamp--
Build failed!
--Some error message--
Compiled successfully --date stamp--

Let me know what you think (also @lukeed @developit @thangngoc89 )

Copy link
Collaborator

Choose a reason for hiding this comment

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

Only show live URL on first render is more than enough for me. And I really hate any watcher that clear my terminal output

Copy link
Member

Choose a reason for hiding this comment

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

I'm good with that format. I believe the "compiled successfully" bit can be done as the completion message of ProgressBarPlugin if it isn't already.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is now kinda different with @reznord's changes. Basically we would now show address only for successful builds and clear console after each build (IMHO that's much nicer).

}

return m;
};

const normalizeName = p => p.substr(p.lastIndexOf('!') + 1);
const stripBabelLoaderPrefix = log => log.replace(/@?\s*(\.\/~\/babel-loader\/lib\?{[\s\S]*?}!)/g, '');
Copy link
Member

Choose a reason for hiding this comment

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

I have a boatload more of these transforms we can add, I'll get clearance to release them shortly.

Choose a reason for hiding this comment

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

Would love to see those boatload of transforms @developit !

compiler.plugin('done', stats => {
if (first) {
Copy link
Member

Choose a reason for hiding this comment

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

Removed this since it is better to clear off the console when there is an error and show the network interface when build is successful (every time)

@reznord
Copy link
Member

reznord commented Jul 10, 2017

@thangngoc89 @lukeed @developit this is good for your review !!

Copy link
Member

@developit developit left a comment

Choose a reason for hiding this comment

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

I'm so ready for this

@reznord reznord merged commit 494eb90 into master Jul 10, 2017
@reznord
Copy link
Member

reznord commented Jul 10, 2017

Wohoo !! Here we go !!

@reznord reznord added this to the 1.4.0 milestone Jul 10, 2017
@reznord reznord mentioned this pull request Jul 16, 2017
@reznord reznord deleted the feat/less-verbose-errors branch July 19, 2017 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants