From 6094369a8f9e35d301cc9f091630bb452aad4343 Mon Sep 17 00:00:00 2001 From: Niklas Mischkulnig <4586894+mischnic@users.noreply.github.com> Date: Mon, 17 Jan 2022 17:40:18 +0100 Subject: [PATCH] Don't discard invalidations when transformer throws an error (#7547) --- packages/core/core/src/Transformation.js | 18 ++++++-- .../core/core/src/requests/AssetRequest.js | 14 ++++-- packages/core/integration-tests/test/cache.js | 43 +++++++++++++++++++ .../parcel-transformer-included/index.js | 3 ++ 4 files changed, 71 insertions(+), 7 deletions(-) diff --git a/packages/core/core/src/Transformation.js b/packages/core/core/src/Transformation.js index 321a327e315..d8e90198d8f 100644 --- a/packages/core/core/src/Transformation.js +++ b/packages/core/core/src/Transformation.js @@ -26,9 +26,11 @@ import {Readable} from 'stream'; import nullthrows from 'nullthrows'; import logger, {PluginLogger} from '@parcel/logger'; import ThrowableDiagnostic, { + anyToDiagnostic, errorToDiagnostic, escapeMarkdown, md, + type Diagnostic, } from '@parcel/diagnostic'; import {SOURCEMAP_EXTENSIONS} from '@parcel/utils'; import {hashString} from '@parcel/hash'; @@ -83,7 +85,8 @@ export type TransformationOpts = {| |}; export type TransformationResult = {| - assets: Array, + assets?: Array, + error?: Array, configRequests: Array, invalidations: Array, invalidateOnFileCreate: Array, @@ -178,19 +181,26 @@ export default class Transformation { asset.value.isSource, asset.value.pipeline, ); - let results = await this.runPipelines(pipeline, asset); - let assets = results.map(a => a.value); + let assets, error; + try { + let results = await this.runPipelines(pipeline, asset); + assets = results.map(a => a.value); + } catch (e) { + error = e; + } let configRequests = getConfigRequests([...this.configs.values()]); let devDepRequests = getWorkerDevDepRequests([ ...this.devDepRequests.values(), ]); - // $FlowFixMe + // $FlowFixMe because of $$raw return { $$raw: true, assets, configRequests, + // When throwing an error, this (de)serialization is done automatically by the WorkerFarm + error: error ? anyToDiagnostic(error) : undefined, invalidateOnFileCreate: this.invalidateOnFileCreate, invalidations: [...this.invalidations.values()], devDepRequests, diff --git a/packages/core/core/src/requests/AssetRequest.js b/packages/core/core/src/requests/AssetRequest.js index 07db6a1e69f..a02541d092e 100644 --- a/packages/core/core/src/requests/AssetRequest.js +++ b/packages/core/core/src/requests/AssetRequest.js @@ -13,6 +13,7 @@ import type {ConfigAndCachePath} from './ParcelConfigRequest'; import type {TransformationResult} from '../Transformation'; import nullthrows from 'nullthrows'; +import ThrowableDiagnostic from '@parcel/diagnostic'; import {hashString} from '@parcel/hash'; import createParcelConfigRequest from './ParcelConfigRequest'; import {runDevDepRequest} from './DevDepRequest'; @@ -128,6 +129,7 @@ async function run({input, api, farm, invalidateReason, options}: RunInput) { let { assets, configRequests, + error, invalidations, invalidateOnFileCreate, devDepRequests, @@ -138,8 +140,10 @@ async function run({input, api, farm, invalidateReason, options}: RunInput) { }): TransformationResult); let time = Date.now() - start; - for (let asset of assets) { - asset.stats.time = time; + if (assets) { + for (let asset of assets) { + asset.stats.time = time; + } } for (let invalidation of invalidateOnFileCreate) { @@ -171,5 +175,9 @@ async function run({input, api, farm, invalidateReason, options}: RunInput) { await runConfigRequest(api, configRequest); } - return assets; + if (error != null) { + throw new ThrowableDiagnostic({diagnostic: error}); + } else { + return nullthrows(assets); + } } diff --git a/packages/core/integration-tests/test/cache.js b/packages/core/integration-tests/test/cache.js index da809e78bb2..667f00566bf 100644 --- a/packages/core/integration-tests/test/cache.js +++ b/packages/core/integration-tests/test/cache.js @@ -5836,6 +5836,49 @@ describe('cache', function () { } }); + it('properly watches included files after a transformer error', async function () { + this.timeout(15000); + let subscription; + let fixture = path.join(__dirname, '/integration/included-file'); + try { + let b = bundler(path.join(fixture, 'index.txt'), { + inputFS: overlayFS, + shouldDisableCache: false, + }); + await overlayFS.mkdirp(fixture); + await overlayFS.writeFile(path.join(fixture, 'included.txt'), 'a'); + subscription = await b.watch(); + let event = await getNextBuild(b); + invariant(event.type === 'buildSuccess'); + let output1 = await overlayFS.readFile( + event.bundleGraph.getBundles()[0].filePath, + 'utf8', + ); + assert.strictEqual(output1, 'a'); + + // Change included file + await overlayFS.writeFile(path.join(fixture, 'included.txt'), 'ERROR'); + event = await getNextBuild(b); + invariant(event.type === 'buildFailure'); + assert.strictEqual(event.diagnostics[0].message, 'Custom error'); + + // Clear transformer error + await overlayFS.writeFile(path.join(fixture, 'included.txt'), 'b'); + event = await getNextBuild(b); + invariant(event.type === 'buildSuccess'); + let output3 = await overlayFS.readFile( + event.bundleGraph.getBundles()[0].filePath, + 'utf8', + ); + assert.strictEqual(output3, 'b'); + } finally { + if (subscription) { + await subscription.unsubscribe(); + subscription = null; + } + } + }); + it('should support moving the project root', async function () { // This test relies on the real filesystem because the memory fs doesn't support renames. // But renameSync is broken on windows in CI with EPERM errors. Just skip this test for now. diff --git a/packages/core/integration-tests/test/integration/included-file/node_modules/parcel-transformer-included/index.js b/packages/core/integration-tests/test/integration/included-file/node_modules/parcel-transformer-included/index.js index e7f12e743cb..31412ca7b7c 100644 --- a/packages/core/integration-tests/test/integration/included-file/node_modules/parcel-transformer-included/index.js +++ b/packages/core/integration-tests/test/integration/included-file/node_modules/parcel-transformer-included/index.js @@ -6,6 +6,9 @@ module.exports = new Transformer({ let file = path.join(path.dirname(asset.filePath), "included.txt"); asset.invalidateOnFileChange(file); let content = await options.inputFS.readFile(file, "utf8"); + if (content === "ERROR") { + throw new Error("Custom error"); + } asset.setCode(content); return [asset]; }