fix(builder): Skip minify of a .js with an existing -dbg.js sibling

packages/builder/lib/processors/minifier.js

@@ -183,6 +183,10 @@ export default async function({
 
 	return Promise.all(resources.map(async (resource) => {
 		const resourcePath = resource.getPath();
+		// TODO: Check whether minifying a -dbg.js is correct here. For such an input this produces a
+		// -dbg-dbg.js debug variant, which is almost certainly unintended. A -dbg.js should probably be
+		// skipped instead. The minify task already skips a -dbg.js whose non-debug .js sibling is present;
+		// a lone -dbg.js still reaches this processor and gets the double suffix.
 		const dbgPath = resourcePath.replace(debugFileRegex, "-dbg$1");
 		const dbgFilename = posixPath.basename(dbgPath);
 

packages/builder/lib/tasks/minify.js

@@ -1,4 +1,7 @@
 import fsInterface from "@ui5/fs/fsInterface";
+import {getDebugName, getNonDebugName} from "../lbt/utils/ModuleName.js";
+import {getLogger} from "@ui5/logger";
+const log = getLogger("builder:tasks:minify");
 
 /**
  * @public
@@ -67,7 +70,37 @@ export default function build({pattern, omitSourceMapResources = false, useInput
 		name: "minify",
 		// One key per matched resource, so a delta build re-minifies only the resources whose inputs
 		// changed. The input source map is read through the step's workspace and thus tracked per step.
-		keys: async ({workspace}) => workspace.byGlob(pattern),
+		keys: async ({workspace}) => {
+			const resources = await workspace.byGlob(pattern);
+
+			// A -dbg.js next to its .js in the sources is a broken input state: the minify task owns the
+			// -dbg.js path and generates it from the .js, so a pre-existing one must not be there. Were both
+			// kept, the .js key would write its generated debug variant to the -dbg.js path, which the -dbg.js
+			// key also writes - two concurrent keys writing one path, which the shared assertDistinctWrite
+			// guard rejects, crashing the build. Rather than crash, skip both members of such a pair: the .js
+			// is left as it is (the existing -dbg.js already serves as its debug variant) and the -dbg.js is
+			// passed through unchanged.
+			const paths = new Set(resources.map((resource) => resource.getPath()));
+			return resources.filter((resource) => {
+				const resourcePath = resource.getPath();
+				const dbgPath = getDebugName(resourcePath);
+				if (dbgPath && paths.has(dbgPath)) {
+					// A .js whose -dbg.js sibling already exists in the sources: skip it
+					log.warn(
+						`Skipping minification of ${resourcePath}: a debug variant ${dbgPath} already ` +
+						`exists next to it. The minify task generates the debug variant itself, so a ` +
+						`pre-existing one is unexpected. Remove ${dbgPath} from the sources to let the task ` +
+						`generate it.`);
+					return false;
+				}
+				const nonDbgPath = getNonDebugName(resourcePath);
+				if (nonDbgPath && paths.has(nonDbgPath)) {
+					// Skip the -dbg.js itself when its non-debug .js sibling is also present
+					return false;
+				}
+				return true;
+			});
+		},
 		each: async (inputResource, {workspace, taskUtil}) => {
 			// Load the minifier (and its worker-pool module graph) lazily, inside the step body, so plan-time
 			// step-name discovery can import this factory module without evaluating that graph. A build whose

packages/builder/test/lib/tasks/minify.integration.js

@@ -437,3 +437,53 @@ test.serial("integration: minify with taskUtil and resources tagged with OmitFro
 	const cleanupTask = taskUtil.registerCleanupTask.getCall(0).args[0];
 	await cleanupTask();
 });
+
+// A -dbg.js next to its .js in the sources is a broken input state: the minify task owns and generates
+// the -dbg.js, so a pre-existing one must not be there. The glob then matches both the .js and the -dbg.js
+// as separate map-step keys, and the .js key would write its generated debug variant to the -dbg.js path
+// that the -dbg.js key also writes - two concurrent keys writing one path, which crashes the build. To
+// stay robust, the .js and that -dbg.js are both skipped. Other .js files are minified as usual.
+test.serial("integration: minify skips a .js with an existing -dbg.js sibling and that -dbg.js",
+	async (t) => {
+		const {reader, writer, workspace} = createWorkspace();
+		const base = "/resources/my/namespace";
+		const content = `function a(x){return x;}\n`;
+
+		// A .js with an existing -dbg.js sibling - both must be skipped
+		await reader.write(resourceFactory.createResource({
+			path: `${base}/withDbg.js`,
+			string: content
+		}));
+		await reader.write(resourceFactory.createResource({
+			path: `${base}/withDbg-dbg.js`,
+			string: content
+		}));
+		// A plain .js without a sibling - must be minified as usual
+		await reader.write(resourceFactory.createResource({
+			path: `${base}/plain.js`,
+			string: content
+		}));
+
+		await runSteps(minify, {
+			workspace,
+			options: {
+				pattern: "/resources/**/*.js"
+			}
+		});
+
+		// The skipped .js and its -dbg.js are left untouched - the task writes neither them nor any derived
+		// resource (no source map, no overwrite of the existing -dbg.js).
+		t.is(await writer.byPath(`${base}/withDbg.js`), null, "Skipped .js is not written");
+		t.is(await writer.byPath(`${base}/withDbg-dbg.js`), null, "Existing -dbg.js is not written");
+		t.is(await writer.byPath(`${base}/withDbg.js.map`), null, "No source map for the skipped .js");
+
+		// The plain .js is minified as usual, producing a minified result, a debug variant and a source map.
+		const minified = await writer.byPath(`${base}/plain.js`);
+		t.truthy(minified, "Plain .js is written");
+		t.is(await minified.getString(), `function a(n){return n}\n${SOURCE_MAPPING_URL}=plain.js.map`,
+			"Plain .js is minified");
+		const dbg = await writer.byPath(`${base}/plain-dbg.js`);
+		t.truthy(dbg, "Debug variant for the plain .js is written");
+		t.is(await dbg.getString(), content, "Debug variant keeps the original content");
+		t.truthy(await writer.byPath(`${base}/plain.js.map`), "Source map for the plain .js is written");
+	});

packages/builder/test/lib/tasks/minify.js

@@ -5,8 +5,12 @@ import runSteps from "../../../lib/tasks/runSteps.js";
 
 test.beforeEach(async (t) => {
 	const sinon = t.context.sinon = sinonGlobal.createSandbox();
+	// Input resources returned by the glob. The minify step reads getPath() on each to detect an existing
+	// -dbg.js sibling; these plain paths have none, so every input is processed.
+	const inputA = t.context.inputA = {getPath: () => "/resources/a.js"};
+	const inputB = t.context.inputB = {getPath: () => "/resources/b.js"};
 	t.context.workspace = {
-		byGlob: sinon.stub().resolves(["resource A", "resource B"]),
+		byGlob: sinon.stub().resolves([inputA, inputB]),
 		write: sinon.stub().resolves()
 	};
 	t.context.taskUtil = {
@@ -23,9 +27,15 @@ test.beforeEach(async (t) => {
 
 	t.context.fsInterfaceStub = sinon.stub().returns("fs interface");
 	t.context.minifierStub = sinon.stub();
+	t.context.log = {
+		warn: sinon.stub()
+	};
 	t.context.minify = await esmock.p("../../../lib/tasks/minify.js", {
 		"@ui5/fs/fsInterface": t.context.fsInterfaceStub,
-		"../../../lib/processors/minifier.js": t.context.minifierStub
+		"../../../lib/processors/minifier.js": t.context.minifierStub,
+		"@ui5/logger": {
+			getLogger: sinon.stub().withArgs("builder:tasks:minify").returns(t.context.log)
+		}
 	});
 });
 test.afterEach.always(async (t) => {
@@ -42,22 +52,22 @@ test.afterEach.always(async (t) => {
 });
 
 test("minify: Default params", async (t) => {
-	const {minify, workspace, taskUtil, minifierStub} = t.context;
+	const {minify, workspace, taskUtil, minifierStub, inputA, inputB} = t.context;
 	// One step per resource: the processor is called once per input resource.
-	const processedByInput = {
-		"resource A": {
+	const processedByInput = new Map([
+		[inputA, {
 			resource: "resource A",
 			dbgResource: "dbgResource A",
 			sourceMapResource: "sourceMapResource A",
 			dbgSourceMapResource: "dbgSourceMapResource A" // optional
-		},
-		"resource B": {
+		}],
+		[inputB, {
 			resource: "resource B",
 			dbgResource: "dbgResource B",
 			sourceMapResource: "sourceMapResource B",
-		}
-	};
-	minifierStub.callsFake(({resources}) => Promise.resolve([processedByInput[resources[0]]]));
+		}]
+	]);
+	minifierStub.callsFake(({resources}) => Promise.resolve([processedByInput.get(resources[0])]));
 	await runSteps(minify, {
 		workspace,
 		taskUtil,
@@ -68,8 +78,8 @@ test("minify: Default params", async (t) => {
 
 	t.is(minifierStub.callCount, 2, "minifier got called once per resource");
 	const minifierCallArgs = minifierStub.firstCall.firstArg;
-	t.deepEqual(minifierCallArgs.resources, ["resource A"], "First step processes the first resource");
-	t.deepEqual(minifierStub.secondCall.firstArg.resources, ["resource B"], "Second step: second resource");
+	t.deepEqual(minifierCallArgs.resources, [inputA], "First step processes the first resource");
+	t.deepEqual(minifierStub.secondCall.firstArg.resources, [inputB], "Second step: second resource");
 	t.is(minifierCallArgs.fs, "fs interface", "Correct fs interface provided to processor");
 	t.is(minifierCallArgs.taskUtil, taskUtil, "Correct taskUtil provided to processor");
 	t.deepEqual(minifierCallArgs.options, {
@@ -108,22 +118,22 @@ test("minify: Default params", async (t) => {
 });
 
 test("minify: omitSourceMapResources: true, useInputSourceMaps: false", async (t) => {
-	const {minify, workspace, taskUtil, minifierStub} = t.context;
+	const {minify, workspace, taskUtil, minifierStub, inputA, inputB} = t.context;
 	// One step per resource: the processor is called once per input resource.
-	const processedByInput = {
-		"resource A": {
+	const processedByInput = new Map([
+		[inputA, {
 			resource: "resource A",
 			dbgResource: "dbgResource A",
 			sourceMapResource: "sourceMapResource A",
 			dbgSourceMapResource: "dbgSourceMapResource A" // optional
-		},
-		"resource B": {
+		}],
+		[inputB, {
 			resource: "resource B",
 			dbgResource: "dbgResource B",
 			sourceMapResource: "sourceMapResource B",
-		}
-	};
-	minifierStub.callsFake(({resources}) => Promise.resolve([processedByInput[resources[0]]]));
+		}]
+	]);
+	minifierStub.callsFake(({resources}) => Promise.resolve([processedByInput.get(resources[0])]));
 	await runSteps(minify, {
 		workspace,
 		taskUtil,
@@ -136,8 +146,8 @@ test("minify: omitSourceMapResources: true, useInputSourceMaps: false", async (t
 
 	t.is(minifierStub.callCount, 2, "minifier got called once per resource");
 	const minifierCallArgs = minifierStub.firstCall.firstArg;
-	t.deepEqual(minifierCallArgs.resources, ["resource A"], "First step processes the first resource");
-	t.deepEqual(minifierStub.secondCall.firstArg.resources, ["resource B"], "Second step: second resource");
+	t.deepEqual(minifierCallArgs.resources, [inputA], "First step processes the first resource");
+	t.deepEqual(minifierStub.secondCall.firstArg.resources, [inputB], "Second step: second resource");
 	t.is(minifierCallArgs.fs, "fs interface", "Correct fs interface provided to processor");
 	t.is(minifierCallArgs.taskUtil, taskUtil, "Correct taskUtil provided to processor");
 	t.deepEqual(minifierCallArgs.options, {
@@ -188,22 +198,22 @@ test("minify: omitSourceMapResources: true, useInputSourceMaps: false", async (t
 });
 
 test("minify: No taskUtil", async (t) => {
-	const {minify, workspace, minifierStub} = t.context;
+	const {minify, workspace, minifierStub, inputA, inputB} = t.context;
 	// One step per resource: the processor is called once per input resource.
-	const processedByInput = {
-		"resource A": {
+	const processedByInput = new Map([
+		[inputA, {
 			resource: "resource A",
 			dbgResource: "dbgResource A",
 			sourceMapResource: "sourceMapResource A",
 			dbgSourceMapResource: "dbgSourceMapResource A" // optional
-		},
-		"resource B": {
+		}],
+		[inputB, {
 			resource: "resource B",
 			dbgResource: "dbgResource B",
 			sourceMapResource: "sourceMapResource B",
-		}
-	};
-	minifierStub.callsFake(({resources}) => Promise.resolve([processedByInput[resources[0]]]));
+		}]
+	]);
+	minifierStub.callsFake(({resources}) => Promise.resolve([processedByInput.get(resources[0])]));
 	await runSteps(minify, {
 		workspace,
 		options: {
@@ -213,8 +223,8 @@ test("minify: No taskUtil", async (t) => {
 
 	t.is(minifierStub.callCount, 2, "minifier got called once per resource");
 	const minifierCallArgs = minifierStub.firstCall.firstArg;
-	t.deepEqual(minifierCallArgs.resources, ["resource A"], "First step processes the first resource");
-	t.deepEqual(minifierStub.secondCall.firstArg.resources, ["resource B"], "Second step: second resource");
+	t.deepEqual(minifierCallArgs.resources, [inputA], "First step processes the first resource");
+	t.deepEqual(minifierStub.secondCall.firstArg.resources, [inputB], "Second step: second resource");
 	t.is(minifierCallArgs.fs, "fs interface", "Correct fs interface provided to processor");
 	t.is(minifierCallArgs.taskUtil, undefined, "No taskUtil provided to processor");
 	t.deepEqual(minifierCallArgs.options, {
@@ -231,3 +241,44 @@ test("minify: No taskUtil", async (t) => {
 		t.is(workspace.write.getCall(idx).firstArg, resName, "workspace#write got called for expected resource");
 	});
 });
+
+test("minify: Skips and warns for an existing -dbg.js next to its .js", async (t) => {
+	const {minify, workspace, taskUtil, minifierStub, log} = t.context;
+	// A .js with an existing -dbg.js sibling (broken input state), plus a plain .js without a sibling.
+	const normal = {getPath: () => "/resources/normal.js"};
+	const withDbg = {getPath: () => "/resources/withDbg.js"};
+	const existingDbg = {getPath: () => "/resources/withDbg-dbg.js"};
+	workspace.byGlob.resolves([normal, withDbg, existingDbg]);
+
+	const processedByInput = new Map([
+		[normal, {
+			resource: "resource normal",
+			dbgResource: "dbgResource normal",
+			sourceMapResource: "sourceMapResource normal",
+		}]
+	]);
+	minifierStub.callsFake(({resources}) => Promise.resolve([processedByInput.get(resources[0])]));
+
+	await runSteps(minify, {
+		workspace,
+		taskUtil,
+		options: {
+			pattern: "**"
+		}
+	});
+
+	// Only the plain .js is minified; the .js with a sibling and that -dbg.js are both skipped.
+	t.is(minifierStub.callCount, 1, "minifier got called once - only for the plain .js");
+	t.deepEqual(minifierStub.firstCall.firstArg.resources, [normal], "minifier processed the plain .js only");
+
+	t.is(log.warn.callCount, 1, "One warning got logged for the existing -dbg.js");
+	t.true(log.warn.firstCall.firstArg.includes("/resources/withDbg.js") &&
+		log.warn.firstCall.firstArg.includes("/resources/withDbg-dbg.js"),
+	"Warning names the .js and its existing debug variant");
+
+	// Neither the skipped .js nor its -dbg.js got written back.
+	t.is(workspace.write.callCount, 3, "workspace#write got called only for the plain .js and its derived resources");
+	["resource normal", "dbgResource normal", "sourceMapResource normal"].forEach((resName, idx) => {
+		t.is(workspace.write.getCall(idx).firstArg, resName, "workspace#write got called for expected resource");
+	});
+});