Errors thrown inside onwarn are ignored with --silent

[filipesilva]

• Rollup Version: 1.1.2
• Operating System (or Browser): Windows
• Node Version: v10.10.0

How Do We Reproduce?

This example is related to @lukastaegert suggestion in #2582 (comment):

If you want an error to be thrown, you can check the provided warning in onwarn and throw an error there.

Create the following files and install rollup using npm/yarn:

// main.js (empty)

// rollup.config.js
export default {
  input: 'main.js',
  output: { format: 'es' },
  onwarn: (warning) => { throw new Error(warning.message) },
  unknownOption: 42,
};

Expected Behavior

Running rollup -c rollup.config.js will throw Error: Unknown input option.
But rollup -c rollup.config.js --silent will complete successfully.

Actual Behavior

Error to be thrown with and without the --silent flag.

[lukastaegert]

If you provide an "onwarn" handler that does nothing, then this is equivalent to using the "--silent" option. Therefore Rollup assumes that when you pass both the "onwarn" handler and the "--silent" option, then your intention is to no longer forward warnings to the "onwarn" handler.

So you are suggesting that "--silent" should become an ignored flag when an "onwarn" handler is passed? I am not sure that this is an improvement.

In the current situation, you could have a strict "onwarn" handler that turns some warnings into errors but still disable it temporarily via CLI flag to debug issues.

[lukastaegert]

Maybe this is more a documentation issue? Improvements are always welcome!

[filipesilva]

My particular usecase is the same as #2582, which we identified causing problems in bazelbuild/rules_nodejs#459.

In our setup we'd like to not have any logging at all, and instead error out on anything that should show a warning.

Just using the onwarn hook to throw errors wasn't sufficient because Rollup still prints messages at the during CLI execution. The --silent flag removes those messages, but also deactivates the onwarn hook which means we can't get the triggered errors.

Is there any way to not have the Rollup CLI logging while keeping usage of onwarn?

[lukastaegert]

Just using the onwarn hook to throw errors wasn't sufficient because Rollup still prints messages at the during CLI execution

I cannot confirm this and this should not be the case. In my own test code, an empty onwarn handler basically is equivalent to using the --silent option. Are you sure there is no logging code inside your onwarn handler? If not, some kind of repro would help to understand this, maybe this is only specific to certain errors.

[filipesilva]

Given the files below:

// main.js
console.log(1);

// rollup.config.js
export default {
  onwarn: () => {},
  input: 'main.js',
  output: {
    file: 'main.output.js',
    format: 'es'
  }
}

Running the rollup CLI over it will print the following:

kamik@RED-X1C6 MINGW64 /d/sandbox/ivy-bundle-size (master)
$ node_modules/.bin/rollup --format es main.js -o main.output.js

main.js → main.output.js...
created main.output.js in 26ms

kamik@RED-X1C6 MINGW64 /d/sandbox/ivy-bundle-size (master)
$ node_modules/.bin/rollup --format es main.js -o main.output.js --silent

kamik@RED-X1C6 MINGW64 /d/sandbox/ivy-bundle-size (master)
$ node_modules/.bin/rollup --config rollup.config.js

main.js → main.output.js...
created main.output.js in 12ms

kamik@RED-X1C6 MINGW64 /d/sandbox/ivy-bundle-size (master)
$ node_modules/.bin/rollup --config rollup.config.js --silent

Rollup will always print the three lines below when not using --silent:

main.js → main.output.js...
created main.output.js in 12ms

[filipesilva]

To be clear, those 3 lines are not errors. They are just logging that can only be removed with --silent.

[lukastaegert]

Ah I see, that was the misunderstanding. So the issue is the double meaning of silent as preventing logging and preventing warnings. Will need to think about this. So there would be value to retaining the onwarn in silent mode which is to remove the default logging. Thanks for clearing this up. I guess in that case your suggestion makes total sense.

[lukastaegert]

Fix at #2981