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

Jump to bottom

rollup_bundle: stop --silent rollup to let it report all build errors #459

Closed
wants to merge 5 commits into from
Closed

rollup_bundle: stop --silent rollup to let it report all build errors #459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
37700e3
update
ashi009 Dec 6, 2018
485a268
fix build
ashi009 Dec 6, 2018
6e43701
remove --experimentalDynamicImport as it's nolonger experimental: htt…
ashi009 Dec 6, 2018
90422e7
fix test
ashi009 Dec 6, 2018
b8c53ac
name plugins and find bazelResolve from that
ashi009 Dec 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 9 additions & 7 deletions internal/rollup/rollup.config.js
View file
Open in desktop
Expand Up @@ -154,26 +154,26 @@ const inputs = [TMPL_inputs];
const enableCodeSplitting = inputs.length > 1;

const config = {
resolveBazel,
banner,
onwarn: (warning) => {
onwarn: ({loc, frame, message}) => {
// Always fail on warnings, assuming we don't know which are harmless.
// We can add exclusions here based on warning.code, if we discover some
// types of warning should always be ignored under bazel.
throw new Error(warning.message);
if (loc) {
throw new Error(`${loc.file} (${loc.line}:${loc.column}) ${message}`);
}
throw new Error(message);
},
plugins: [TMPL_additional_plugins].concat([
{resolveId: resolveBazel},
{name: 'bazel-resolve', resolveId: resolveBazel},
nodeResolve(
{jsnext: true, module: true, customResolveOptions: {moduleDirectory: nodeModulesRoot}}),
{resolveId: notResolved},
{name: 'bazel-not-resolved', resolveId: notResolved},
sourcemaps(),
])
}

if (enableCodeSplitting) {
config.experimentalCodeSplitting = true;
config.experimentalDynamicImport = true;
config.input = inputs;
config.output = {
format: 'TMPL_output_format',
Expand All @@ -190,4 +190,6 @@ else {
};
}

config.output.banner = banner;

module.exports = config;
5 changes: 3 additions & 2 deletions internal/rollup/rollup.config.spec.js
View file
Open in desktop
Expand Up @@ -31,9 +31,10 @@ const resolve =
}

const rollupConfig = require('./rollup.config');
const resolveBazel = rollupConfig.plugins.find(({name}) => name === 'bazel-resolve').resolveId;

function doResolve(importee, importer) {
const resolved = rollupConfig.resolveBazel(importee, importer, baseDir, resolve, rootDir);
const resolved = resolveBazel(importee, importer, baseDir, resolve, rootDir);
if (resolved) {
return resolved.replace(/\\/g, '/');
} else {
Expand Down Expand Up @@ -72,7 +73,7 @@ describe('rollup config', () => {
expect(doResolve(`other${sep}thing`))
.toEqual(`${baseDir}/bazel-bin/path/to/a.esm5/external/other_wksp/path/to/other_lib/thing`);
expect(doResolve(`@bar${sep}baz${sep}foo`))
.toEqual(`${baseDir}/bazel-bin/path/to/a.esm5/path/to/bar/baz_lib/foo`);
.toEqual(`${baseDir}/bazel-bin/path/to/a.esm5/path/to/bar/baz_lib/foo`);
});

it('should find paths in any root', () => {
Expand Down
5 changes: 4 additions & 1 deletion internal/rollup/rollup_bundle.bzl
View file
Open in desktop
Expand Up @@ -169,7 +169,10 @@ def _run_rollup(ctx, sources, config, output, map_output = None):

# We will produce errors as needed. Anything else is spammy: a well-behaved
# bazel rule prints nothing on success.
args.add("--silent")
#
# TODO(https://github.com/rollup/rollup/issues/2582): Uncomment the following
# line once rollup stops reporting build errors with onwarn.
# args.add("--silent")
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


if ctx.attr.globals:
args.add("--external")
Expand Down