rollup_bundle: stop --silent rollup to let it report all build errors #459
Conversation
There was a problem hiding this comment.
LGTM. @alexeagle any final comments before we merge? // cc @gregmagolan
| # | ||
| # TODO(https://github.com/rollup/rollup/issues/2582): Uncomment the following | ||
| # line once rollup stops reporting build errors with onwarn. | ||
| # args.add("--silent") |
There was a problem hiding this comment.
my concern is what does an execution of bazel look like now? Does rollup print a bunch of stuff to the terminal? as said in the original comment, a well-behaved bazel rule prints nothing on success. Can we somehow throw away the stdout from rollup if the compilation is successful?
There was a problem hiding this comment.
Totally understand. However, in this case I'd prioritize the correctness over the noise made by rollup. OTOH, this is actually inevitable in the nodejs world, even if the rollup is silenced, some poorly written plugin may console.log something from nowhere . So I think we can replace this comment with a more descriptive message pointing to a bug in rules_nodejs saying we should fix the log spam in the future. What do you think?
There was a problem hiding this comment.
Alternatively, we can write a wrapper tool to filter the messages -- but does it worth the effort? As rollup is almost certainly the last step in a release cycle. So it won't be run that frequently and the spam it made should not be that bad.
|
PTAL @alexeagle |
|
Now we can use |
|
Not planning to add features to old rollup-bundle rule, will be deleted by 1.0 you were right though, |
This CL removes
--silentflag when runningrollupto let it report all the warnings and errors (due to rollup/rollup#2582) during the build. The warning message now contains location info when available.As a result of this change, some config errors are found and fixed as well:
config.output.bannerinstead ofconfig.banneras the latter is deprecated in rollup/rollup@ffe4e5b.config.experimentalDynamicImportas rollup/rollup@4d4ec4c made it default and removed it.resolveBazelinconfigto avoid triggering unknown config error, and let the test to find it in plug-in list by name.Updates #447