liferay / liferay-frontend-projects

A monorepo containing assorted Frontend Infrastructure Team projects
Other
69 stars 68 forks source link

Show some esential logging by default #552

Closed izaera closed 3 years ago

izaera commented 3 years ago

Issue type (mark with x)

Version (mark with x)

Description

Based on this conversation decide what to log and how.

izaera commented 3 years ago

I'm going to add an example of report and console output in this issue so that we have some starting point for this.

izaera commented 3 years ago

Sample report: https://gist.github.com/izaera/95fb8b020c69156df0e9b50a8143384c (you may need to download it to be able to open it in the browser)

izaera commented 3 years ago

This is the initial section of the report:

image

Some of this info could probably go to the console as well (for example the preset/platform version and name and, maybe, the bundled packages).

izaera commented 3 years ago

Then we have things like these:

image

That we would all like to see in the console when they cause errors but, unfortunately, so many of them appear that it would be useless :-(

izaera commented 3 years ago

And, finally, these:

image

Which are things we know will fail if executed and should probably go to the console too.

Note, however, that some of these errors may be in files which are never executed so it is also possible that we show error messages to the user that won't be affecting them. Thus, we kept them in the report and never showed them to the users to avoid misleading them.

In any case, when something fails they are instructed to create the report file and they see them. Also, notice how these errors have an 🛈 next to them. That's a link to the detailed explanations table where we can teach the users about the real implications of the error and what to expect.

Adding a message like that to the report it's as easy as doing this in the bundler's code.

izaera commented 3 years ago

There's also a filter in the top right corner:

image

That lets users filter messages by severity (this is useful for showing only errors or warns, for example).

izaera commented 3 years ago

The console logs (with verbose: true) print this for a clean run of a React project:

Copying package '/'...
Copying package 'react@16.8.6'...
Copying package 'loose-envify@1.4.0'...
Copying package 'js-tokens@4.0.0'...
Copying package 'object-assign@4.1.1'...
Copying package 'prop-types@15.7.2'...
Copying package 'react-is@16.13.1'...
Copying package 'scheduler@0.13.6'...
Copying package 'react-dom@16.8.6'...
Copied 7 packages
Applying rules to package '/'...
Applying rules to package 'loose-envify@1.4.0'...
Applying rules to package 'js-tokens@4.0.0'...
Applying rules to package 'object-assign@4.1.1'...
Applying rules to package 'prop-types@15.7.2'...
Applying rules to package 'react-is@16.13.1'...
Applying rules to package 'scheduler@0.13.6'...
Applied rules to 7 packages
Transforming package '/'...
Transforming package 'loose-envify@1.4.0'...
Transforming package 'js-tokens@4.0.0'...
Transforming package 'object-assign@4.1.1'...
Transforming package 'prop-types@15.7.2'...
Transforming package 'react-is@16.13.1'...
Transforming package 'scheduler@0.13.6'...
Babelifying 2 files in package '/'...
Babelifying 5 files in package 'loose-envify@1.4.0'...
Babelifying 1 files in package 'js-tokens@4.0.0'...
Babelifying 1 files in package 'object-assign@4.1.1'...
Babelifying 8 files in package 'prop-types@15.7.2'...
Babelifying 5 files in package 'react-is@16.13.1'...
Babelifying 14 files in package 'scheduler@0.13.6'...
Transformed package '/'
Transformed package 'js-tokens@4.0.0'
Transformed package 'object-assign@4.1.1'
Transformed package 'loose-envify@1.4.0'
Transformed package 'prop-types@15.7.2'
Transformed package 'react-is@16.13.1'
Transformed package 'scheduler@0.13.6'
Transformed 7 packages
Bundling took 1s
Report written to /home/ivan/Temporal/toolkit-workspace/packages/react-widget/liferay-npm-bundler-report.html

The next time it is run, because the build of all packages in node_modules is cached, the logs show this:

Copying package '/'...
Copying package 'react@16.8.6'...
Copying package 'react-dom@16.8.6'...
Copied 1 packages
Applying rules to package '/'...
Applied rules to 1 packages
Transforming package '/'...
Babelifying 2 files in package '/'...
Transformed package '/'
Transformed 1 packages
Bundling took 253ms
Report written to /home/ivan/Temporal/toolkit-workspace/packages/react-widget/liferay-npm-bundler-report.html

I tend to see this output useful. Especially the differences between the first and subsequent runs that hint users about what was cached and what didn't, and explains why the build times are so different.

But I admit that it may be too much to dump every bundled package. Not sure...

izaera commented 3 years ago

Maybe we can log just the summaries and the preset/target platform :thinking: . So this:

Using preset: liferay-7.4-ga1
Copied 7 packages
Applied rules to 7 packages
Transformed 7 packages
Bundling took 1s
Report written to /home/ivan/Temporal/toolkit-workspace/packages/react-widget/liferay-npm-bundler-report.html

Or something along those lines...

bryceosterhaus commented 3 years ago

I like your summary log for what to put in the normal console, we may want to add the words bundler and js in there for people without context. Something like "JS Bundler preset:". Also, would it make sense to nest the subsequent lines? This might help see that its all under the bundler task, especially when it's next to other gradlew deploy outputs

JS Bundler preset: liferay-7.4-ga1
    Copied 7 packages
    Applied rules to 7 packages
    Transformed 7 packages
    Bundling took 1s
    Report written to /home/ivan/Temporal/toolkit-workspace/packages/react-widget/liferay-npm-bundler-report.html

The verbose one looks good. And then for the exported report, it's pretty dense and I don't know if many people would check that out unless it was a last resort. For the report, do we always dump that or only when a specific config is applied? It might be nice to add it behind a flag since it probably won't be used often.

Overall though, I think these look pretty good and we can always add/remove items from the log as needed.

izaera commented 3 years ago

Also, would it make sense to nest the subsequent lines?

Looks better, yes. We may also add emojis and/or colors, which make readability better in terminals. We have a small utility for that. It also lets users disable colors and formatting with an env variable, in case they don't want/can't see it for any reason.

izaera commented 3 years ago

for the exported report, it's pretty dense and I don't know if many people would check that out unless it was a last resort

Yeah, the intention of the report is to make an exhaustive explanation of the build for:

  1. learning purposes
  2. diagnosing problems with bundled code

I don't think anybody is going to read a full report, because it's huge, and some of the things that are shown there may not be relevant because you are not executing them.

There could be a specific use case we could extract to some other place (maybe we could set verbose to be a level instead of a boolean for that): warnings and errors. There's the filter for that in the report file, but maybe it could be useful if we dumped warning and/or errors directly to the console on user's request, as some other tools allow you to do.

Not sure really, though... Nobody has asked for it ever, so it might be that the current scheme of things is enough. Also, there's no way to diminish the number of warning and errors in a build since they are normally caused by dependencies and you don't have control over them, so it is probable that it may be better to leave it in the report as it is now :shrug: .

bryceosterhaus commented 3 years ago

I think it's fine to keep the report and it might help debugging in the future for odd cases. And I am also happy with either putting that behind a config flag or a --verbose level. The most important is the default logs and those look great to me

izaera commented 3 years ago

I'm closing this as it seems settled and we can always polish what we don't like in the final review.