Propagate reporter errors

Author: MonicaOlejniczak

packages/core/core/src/Parcel.js

@@ -152,8 +152,8 @@ export default class Parcel {
     this.#disposable.add(() => this.#watchEvents.dispose());
 
     this.#reporterRunner = new ReporterRunner({
-      config: this.#config,
       options: resolvedOptions,
+      reporters: await this.#config.getReporters(),
       workerFarm: this.#farm,
     });
     this.#disposable.add(this.#reporterRunner);
@@ -313,7 +313,7 @@ export default class Parcel {
       if (options.shouldTrace) {
         tracer.enable();
       }
-      this.#reporterRunner.report({
+      await this.#reporterRunner.report({
         type: 'buildStart',
       });
 
@@ -401,6 +401,11 @@ export default class Parcel {
         createValidationRequest({optionsRef: this.#optionsRef, assetRequests}),
         {force: assetRequests.length > 0},
       );
+
+      if (this.#reporterRunner.errors.length) {
+        throw this.#reporterRunner.errors;
+      }
+
       return event;
     } catch (e) {
       if (e instanceof BuildAbortError) {

packages/core/core/src/ReporterRunner.js

@@ -12,7 +12,6 @@ import {
   NamedBundle,
 } from './public/Bundle';
 import WorkerFarm, {bus} from '@parcel/workers';
-import ParcelConfig from './ParcelConfig';
 import logger, {
   patchConsole,
   unpatchConsole,
@@ -24,23 +23,24 @@ import BundleGraph from './BundleGraph';
 import {tracer, PluginTracer} from '@parcel/profiler';
 
 type Opts = {|
-  config: ParcelConfig,
   options: ParcelOptions,
+  reporters: Array<LoadedPlugin<Reporter>>,
   workerFarm: WorkerFarm,
 |};
 
 const instances: Set<ReporterRunner> = new Set();
 
 export default class ReporterRunner {
   workerFarm: WorkerFarm;
-  config: ParcelConfig;
+  errors: Error[];
   options: ParcelOptions;
   pluginOptions: PluginOptions;
   reporters: Array<LoadedPlugin<Reporter>>;
 
   constructor(opts: Opts) {
-    this.config = opts.config;
+    this.errors = [];
     this.options = opts.options;
+    this.reporters = opts.reporters;
     this.workerFarm = opts.workerFarm;
     this.pluginOptions = new PluginOptions(this.options);
 
@@ -86,40 +86,33 @@ export default class ReporterRunner {
   };
 
   async report(event: ReporterEvent) {
-    // We should catch all errors originating from reporter plugins to prevent infinite loops
-    try {
-      let reporters = this.reporters;
-      if (!reporters) {
-        this.reporters = await this.config.getReporters();
-        reporters = this.reporters;
-      }
-
-      for (let reporter of this.reporters) {
-        let measurement;
-        try {
-          // To avoid an infinite loop we don't measure trace events, as they'll
-          // result in another trace!
-          if (event.type !== 'trace') {
-            measurement = tracer.createMeasurement(reporter.name, 'reporter');
-          }
-          await reporter.plugin.report({
-            event,
-            options: this.pluginOptions,
-            logger: new PluginLogger({origin: reporter.name}),
-            tracer: new PluginTracer({
-              origin: reporter.name,
-              category: 'reporter',
-            }),
-          });
-        } catch (reportError) {
+    for (let reporter of this.reporters) {
+      let measurement;
+      try {
+        // To avoid an infinite loop we don't measure trace events, as they'll
+        // result in another trace!
+        if (event.type !== 'trace') {
+          measurement = tracer.createMeasurement(reporter.name, 'reporter');
+        }
+        await reporter.plugin.report({
+          event,
+          options: this.pluginOptions,
+          logger: new PluginLogger({origin: reporter.name}),
+          tracer: new PluginTracer({
+            origin: reporter.name,
+            category: 'reporter',
+          }),
+        });
+      } catch (reportError) {
+        if (event.type !== 'buildSuccess') {
+          // This will be captured by consumers
           INTERNAL_ORIGINAL_CONSOLE.error(reportError);
-          process.exitCode = 1;
-        } finally {
-          measurement && measurement.end();
         }
+
+        this.errors.push(reportError);
+      } finally {
+        measurement && measurement.end();
       }
-    } catch (err) {
-      INTERNAL_ORIGINAL_CONSOLE.error(err);
     }
   }
 

packages/core/integration-tests/test/integration/reporters-load-failure/.parcelrc

@@ -0,0 +1,4 @@
+{
+  "extends": "@parcel/config-default",
+  "reporters": ["./test-reporter"]
+}

packages/core/integration-tests/test/integration/reporters-load-failure/index.js

@@ -0,0 +1 @@
+export function main() {}

packages/core/integration-tests/test/integration/reporters-load-failure/yarn.lock

@@ -4,9 +4,7 @@ import assert from 'assert';
 import {execSync} from 'child_process';
 import path from 'path';
 
-import {INTERNAL_ORIGINAL_CONSOLE} from '@parcel/logger';
-import {bundle} from '@parcel/test-utils';
-import sinon from 'sinon';
+import {bundler} from '@parcel/test-utils';
 
 describe('reporters', () => {
   let successfulEntry = path.join(
@@ -16,7 +14,14 @@ describe('reporters', () => {
     'index.js',
   );
 
-  let failingEntry = path.join(
+  let loadReporterFailureEntry = path.join(
+    __dirname,
+    'integration',
+    'reporters-load-failure',
+    'index.js',
+  );
+
+  let failingReporterEntry = path.join(
     __dirname,
     'integration',
     'reporters-failure',
@@ -32,42 +37,56 @@ describe('reporters', () => {
       );
     });
 
-    it('exit with an error code when an error is emitted', () => {
+    it('exit with an error code when a reporter fails to load', () => {
       assert.throws(() =>
-        execSync(`parcel build --no-cache ${failingEntry}`, {stdio: 'ignore'}),
+        execSync(`parcel build --no-cache ${loadReporterFailureEntry}`, {
+          stdio: 'ignore',
+        }),
+      );
+    });
+
+    it('exit with an error code when a reporter emits an error', () => {
+      assert.throws(() =>
+        execSync(`parcel build --no-cache ${failingReporterEntry}`, {
+          stdio: 'ignore',
+        }),
       );
     });
   });
 
   describe('running on the programmatic api', () => {
-    let consoleError;
-    let processExitCode;
+    it('resolves when no errors are emitted', async () => {
+      let buildEvent = await bundler(successfulEntry).run();
 
-    beforeEach(() => {
-      processExitCode = process.exitCode;
-      consoleError = sinon.stub(INTERNAL_ORIGINAL_CONSOLE, 'error');
+      assert.equal(buildEvent.type, 'buildSuccess');
     });
 
-    afterEach(() => {
-      process.exitCode = processExitCode;
-      sinon.restore();
-    });
-
-    it('exit successfully when no errors are emitted', async () => {
-      await bundle(successfulEntry);
+    it('rejects when a reporter fails to load', async () => {
+      try {
+        let buildEvent = await bundler(loadReporterFailureEntry).run();
 
-      assert(!process.exitCode);
+        throw new Error(buildEvent);
+      } catch (err) {
+        assert.equal(err.name, 'Error');
+        assert.deepEqual(
+          err.diagnostics.map(d => d.message),
+          ['Cannot find Parcel plugin "./test-reporter"'],
+        );
+      }
     });
 
-    it('exit with an error code when an error is emitted', async () => {
-      await bundle(failingEntry);
+    it('rejects when a reporter emits an error', async () => {
+      try {
+        let buildEvent = await bundler(failingReporterEntry).run();
 
-      assert.equal(process.exitCode, 1);
-      assert(
-        consoleError.calledWithMatch({
-          message: 'Failed to report buildSuccess',
-        }),
-      );
+        throw new Error(buildEvent);
+      } catch (err) {
+        assert.equal(err.name, 'BuildError');
+        assert.deepEqual(
+          err.diagnostics.map(d => d.message),
+          ['Failed to report buildSuccess'],
+        );
+      }
     });
   });
 });

packages/core/integration-tests/test/reporters.js

@@ -404,7 +404,7 @@ async function run(
       await exit(1);
     }
 
-    await exit(process.exitCode);
+    await exit();
   }
 }