feat(babel-cli): add a brief summary to build output by thymikee · Pull Request #7439 · babel/babel

thymikee · 2018-02-27T11:57:36Z

Q	A
Fixed Issues?	Fixes #6303
Patch: Bug Fix?	No
Major: Breaking Change?	No
Minor: New Feature?	Yes
Tests Added + Pass?	Yes
Documentation PR	No
Any Dependency Changes?	No
License	MIT

Adds a brief summary when running babel-cli. It displays even when --quiet flag is passed.

cc @hzoo @Andarist

thymikee · 2018-02-27T11:58:57Z

packages/babel-cli/src/babel/dir.js

      function(err, res) {
-        if (err) return callback(err);
+        if (err) {
+          result.failure += 1;


For now I gather failures count, but don't use them anywhere. Happy to remove this.

what happens when error occurs here? i suspect it crash whole process, right? in other words - can failure increase beyond 1 here?

It forwards the error object and crashes here with non-zero exit code. I don't use failures anywhere now (I used to show number of failed files but decided it's not necessary, as usually babel just crash the whole thing)

we could remove counting failure then and convert result to a "flat" counter of successes, would give future readers a clear purpose for this, with unused failures it might be a little bit unclear

thymikee · 2018-02-27T12:01:16Z

packages/babel-cli/src/babel/dir.js


    handle(filename, function(err) {
-      if (err) throw err;
+      if (err) throw err.toString();


Since the stack trace of build errors isn't very helpful (lots of internal Babel stuff not necessarily helpful to the end user), I've decided to use only the stringified message. It works well on Node >=8. Node <=6 produces a totally unhelpful message anyway 🤷‍♂️

Is it a common practice in other CLI tools? is this error here crashing the process directly & going right into stderr?

Yup, crashes entire thing. Throwing err.toString() is definitely not something common, usually you end up creating your own error extending from Error and override stacktrace if necessary, for better error messages.

so if this is direct exit point of the program & to avoid throwing non error it could be converted to smth like:

console.error(err.toString()) process.exit(1)

although im not entirely convinced that this is a good change, logged value is still logged currently, right? Only with less-readable for most users stack trace. Personally I often take such stack traces and go into my node_modules to get better understanding of what happened if the error message is not descriptive enough (which is most of the times 😉)

I just noticed we throw raw error without wrapping it with an Error object. I've pushed a change to adjust that

Here's how it looks now:

imho better than:

Definitely better!

thymikee · 2018-02-27T12:03:51Z

packages/babel-cli/test/fixtures/babel/--copy-files --include-dotfiles with ignore/stdout.txt

@@ -1 +1,2 @@
 src/index.js -> lib/index.js
+Successfully compiled 1 file.


I thought about putting this into stderr instead of stdout (I wouldn't need to adjust all tests, just add a new one), but based on experience in Jest CLI, I'd rather avoid that, as users tend to abuse this stream for catching their own errors or such.

stdout is imho a perfect place for this, stderr implies some kind of error, warning at least, and this is a regular info message

babel-bot · 2018-02-27T12:08:55Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/7043/

Andarist · 2018-02-27T22:22:56Z

packages/babel-cli/src/babel/util.js


-export function log(msg) {
-  if (!commander.quiet) console.log(msg);
+export function log(msg, alwaysLog) {


nit picking: i think always would suffice here

Andarist · 2018-02-27T22:54:23Z

packages/babel-cli/src/babel/dir.js


    handle(filename, function(err) {
-      if (err) throw err;
+      if (err) throw Error(err);


this probably should be a new Error

they're equivalent https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error

While they might be equivalent, it's most commonly used as a constructor - I'd prefer to new it for consistency.

Andarist

Thanks for the PR and putting up with my nit picking.

existentialism · 2018-02-28T14:59:05Z

Just spitballing, but any thoughts on prefixing the message (and maybe all other logs/errors from Babel)?

Babel: 🎉 Successfully compiled 1,492 files.

Might help with searching/parsing logs, or helping users understand where errors are coming from? Just thinking aloud :)

thymikee · 2018-02-28T15:35:42Z

I was thinking of adding an emoji too! But it looked weird with Yarn output (unprefixed), because they use 2 spaces:

🎉 Successfully compiled 1,492 files.
✨  Done in 0.72s.

Not sure about prefixing though, it's not something I've seen in the wild tbh.

How about this (also aligning with Yarn):

🎉  Successfully compiled 1,492 files with Babel.
✨  Done in 0.72s.

Andarist · 2018-03-01T21:53:54Z

packages/babel-preset-env/test/debug-fixtures/usage/stdout.txt

  web.dom.iterable { "chrome":"52", "firefox":"50", "ie":"11" }
-src/in2.js -> lib/in2.js
+src/in2.js -> lib/in2.js
+🎉  successfully compil


this seems off, the line got splitted into 2, some other stdout.txt suffer from the same thing, any idea why this happens?

huh, that's just my regex not playing the way I liked. Weird that test didn't caught it (at least locally)?

Oh, I was scoping my testing to babel-cli only, I'll update this shortly

Andarist · 2018-03-01T21:55:45Z

packages/babel-cli/src/babel/index.js

 delete opts.copyFiles;
 delete opts.includeDotfiles;
-delete opts.quiet;
+delete opts.verbose;


was this change requested somewhere? whats the motivation behind it?

discussed here: #6303 (comment)

thymikee · 2018-03-01T22:13:58Z

npm:

yarn:

Andarist · 2018-03-01T22:15:32Z

The only thing missing (I think) is like at least a single test covering default mode, right? (i dont see any test converted from previously quiet to now default)

thymikee · 2018-03-01T22:16:51Z

packages/babel-cli/test/fixtures/babel/dir --out-dir/stdout.txt

@@ -1,2 +1 @@
-src/bar/bar.js -> lib/bar/bar.js
-src/foo.js -> lib/foo.js
+🎉  Successfully compiled 2 files with Babel.


@Andarist, here is the default without --verbose flag

👍 disregard my comment then

thymikee · 2018-03-01T22:17:36Z

packages/babel-preset-env/test/debug-fixtures/android/stdout.txt

  web.immediate { "android":"4" }
  web.dom.iterable { "android":"4" }
-src/in.js -> lib/in.js
+🎉  Successfully compiled 1 file with Babel.


Also, all tests in babel-preset-env cover default quiet mode.

thymikee · 2018-03-01T22:33:41Z

Travis failure seems unrelated.

existentialism · 2018-03-01T22:43:46Z

@thymikee it almost certainly is, those particular tests have been finicky lately, haven't had a chance to look into it much. restarted it tho!

thymikee · 2018-03-01T22:50:14Z

packages/babel-cli/src/babel/index.js

  "Include dotfiles when compiling and copying non-compilable files",
 );
-commander.option("-q, --quiet", "Don't log anything");
+commander.option("-v, --verbose", "Log everything");


Also, what do you think about the alias – should it be -v or maybe no alias at all? At least it doesn't collide with anything right now.

Oh, havent spotted that yesterday - I think verbose is not like the most wanted/common thing and it might not deserve alias just yet, especially that -v is most commonly used for version in other tools.

* feat(babel-cli): add a brief summary to build output * address feedback * further adjustments * Use quiet output as default, add --verbose * fix tests * remove verbose alias

xtuc · 2018-07-02T05:32:23Z

Sorry I missed this PR and it's a while ago. I noticed that the flag --quiet has been renamed, people that relied on that flag will now get an error? It's a breaking change.

Also we'd like to remove the emojis #8250.

thymikee · 2018-07-02T06:48:45Z

I noticed that the flag --quiet has been renamed, people that relied on that flag will now get an error? It's a breaking change.

Not sure about how CLI handles unknown flags passed to it. If it errors, then it's a breaking change – to unbreak it, we could add the --quiet flag back which would do nothing at the moment, and be there for future updates.

Andarist · 2018-07-02T10:04:48Z

As removal of --quiet was merged only to beta line, I would assume it's OK for it to be a breaking change. If we do not need it right now then it doesn't seem necessary to add it only to avoid a breaking change in this situation. We might want to document this change though as breaking one.

xtuc · 2018-07-02T17:52:52Z

I tried using 7.0.0-beta.51 (@babel/core 7.0.0-beta.51), passing an unknown flag returns an error and an exit code 1. It's a breaking change, even if it's since Babel 7 we should communicate accordingly.

IMO we should rather not introduce unnecessary breaking changes.

feat(babel-cli): add a brief summary to build output

ed6291a

thymikee commented Feb 27, 2018

View reviewed changes

existentialism added PR: New Feature 🚀 A type of pull request used for our changelog categories pkg: cli labels Feb 27, 2018

Andarist reviewed Feb 27, 2018

View reviewed changes

address feedback

fecf7e6

Andarist reviewed Feb 27, 2018

View reviewed changes

further adjustments

a291f78

Andarist approved these changes Feb 28, 2018

View reviewed changes

Use quiet output as default, add --verbose

24790f8

Andarist reviewed Mar 1, 2018

View reviewed changes

fix tests

94f5990

thymikee commented Mar 1, 2018

View reviewed changes

remove verbose alias

99b4d58

existentialism approved these changes Mar 2, 2018

View reviewed changes

Andarist merged commit cc6e739 into babel:master Mar 3, 2018

dependabot-preview bot mentioned this pull request Mar 14, 2018

Bump @babel/register from 7.0.0-beta.37 to 7.0.0-beta.41 nju33/yosuga#189

Closed

thymikee deleted the feat/babel-cli-output branch March 15, 2018 17:12

jaydenseric mentioned this pull request May 14, 2018

No babel-cli watch mode console output after first compilation #7926

Closed

insin mentioned this pull request Jul 14, 2018

@babel/cli can't be quietened #8326

Closed

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019

		@@ -1 +1,2 @@
		src/index.js -> lib/index.js
		Successfully compiled 1 file.

Uh oh!

Conversation

thymikee commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

babel-bot commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andarist left a comment

Choose a reason for hiding this comment

Uh oh!

existentialism commented Feb 28, 2018

Uh oh!

thymikee commented Feb 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thymikee commented Mar 1, 2018

Uh oh!

Andarist commented Mar 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thymikee commented Mar 1, 2018

Uh oh!

existentialism commented Mar 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

thymikee commented Feb 27, 2018 •

edited

Loading

babel-bot commented Feb 27, 2018 •

edited

Loading