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.

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

cli: implement --failAfterWarnings flag #3712

Merged
merged 11 commits into from
Aug 14, 2020
1 change: 1 addition & 0 deletions cli/help.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ Basic options:
--preserveSymlinks Do not follow symlinks when resolving files
--shimMissingExports Create shim variables for missing exports
--silent Don't print warnings
--failAfterWarnings Exit with an error code if there was a warning during the build
Copy link
Member

Choose a reason for hiding this comment

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

We also need to extend documentation in the docs folder, just check where and how e.g. --enivronment is documented.

--sourcemapExcludeSources Do not include source code in source maps
--sourcemapFile <file> Specify bundle position for source maps
--no-stdin do not read "-" from stdin
Expand Down
7 changes: 7 additions & 0 deletions cli/run/batchWarnings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,27 @@ import { stderr } from '../logging';

export interface BatchWarnings {
add: (warning: RollupWarning) => void;
readonly total: number;
readonly count: number;
flush: () => void;
}

export default function batchWarnings() {
let deferredWarnings = new Map<keyof typeof deferredHandlers, RollupWarning[]>();
let total = 0;
let count = 0;

return {
get total() {
return total;
},

get count() {
return count;
},

add: (warning: RollupWarning) => {
total += 1;
tjenkinson marked this conversation as resolved.
Show resolved Hide resolved
count += 1;

if (warning.code! in deferredHandlers) {
Expand Down
6 changes: 6 additions & 0 deletions cli/run/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,12 @@ export default async function runRollup(command: any) {
for (const inputOptions of options) {
await build(inputOptions, warnings, command.silent);
}
if (command.failAfterWarnings && warnings.total) {
throw {
code: 'FAIL_AFTER_WARNINGS',
message: 'Warnings occurred and --failAfterWarnings flag present'
};
tjenkinson marked this conversation as resolved.
Show resolved Hide resolved
}
} catch (err) {
warnings.flush();
handleError(err);
Expand Down
1 change: 1 addition & 0 deletions src/utils/options/mergeOptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export function mergeOptions(
'environment',
'plugin',
'silent',
'failAfterWarnings',
'stdin',
'waitForBundleInput'
),
Expand Down
9 changes: 9 additions & 0 deletions test/cli/samples/fail-after-warnings/_config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
const { assertIncludes } = require('../../../utils.js');

module.exports = {
description: 'errors on warnings with --failAfterWarnings',
command: 'rollup -i main.js --failAfterWarnings',
error: () => true,
stderr: stderr =>
assertIncludes(stderr, '[!] Warnings occurred and --failAfterWarnings flag present')
};
5 changes: 5 additions & 0 deletions test/cli/samples/fail-after-warnings/_expected.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
'use strict';

require('unknown');
Copy link
Member Author

Choose a reason for hiding this comment

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

don't think this is been checked with error: () => true in this or other tests. will investigate

Copy link
Member

Choose a reason for hiding this comment

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

Please do. I checked e.g. node-config-not-found and changing the expected output there turned the test red.

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case it's because I wasn't returning true to make it continue. In the other tests where _expected.js was being ignored I removed _expected.js because they didn't match, and I'm not sure if the intent was to check the output or not?

Copy link
Member

Choose a reason for hiding this comment

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

You need to return true in order to continue

if (!shouldContinue) return done();

_expected.js should be compared to stdout, but only if there is no _expected dir, config.test, config.result, config.execute, config.stderr that does not return true or config.error that does not return true.


console.log(42);
3 changes: 3 additions & 0 deletions test/cli/samples/fail-after-warnings/main.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import something from 'unknown';

console.log(42);