From c5dd5314186621f5b6a39db4ded84b31d8c84b53 Mon Sep 17 00:00:00 2001 From: Matthias Osswald Date: Tue, 13 Oct 2026 14:59:53 +0200 Subject: [PATCH] fix(builder): Skip minify of a .js with an existing -dbg.js sibling A -dbg.js next to its .js in the sources is unexpected: the minify task owns the -dbg.js path and generates it from the .js. The glob matched both as separate map-step keys. The .js key wrote its generated debug variant to the -dbg.js path, which the -dbg.js key also wrote - two concurrent keys writing one path, which the shared assertDistinctWrite guard rejected, so the build crashed. To stay robust rather than crash, filter the step keys to skip both members of such a pair: a .js whose -dbg.js sibling already exists is left as it is (that -dbg.js already serves as the debug variant), the -dbg.js is passed through, and a warning is logged. The sibling paths are derived with the shared getDebugName and getNonDebugName helpers, so the special .view.js / .controller.js suffixes are handled consistently. The warning states that sources normally must not contain a -dbg.js, but if the pairing is intentional (for example the file uses ECMAScript syntax the minify task does not support yet), the user should list both files in the "builder.minification.excludes" configuration of the project's ui5.yaml. The integration test reproduces the crash and pins the expected skip; unit tests cover the filter and the warning. Also add a TODO in the minify processor: a lone -dbg.js (no .js sibling) still reaches the processor and gets a -dbg-dbg.js debug variant, which is almost certainly unintended. Clarify whether such files should be skipped instead. --- packages/builder/lib/processors/minifier.js | 4 + packages/builder/lib/tasks/minify.js | 36 +++++- .../test/lib/tasks/minify.integration.js | 50 ++++++++ packages/builder/test/lib/tasks/minify.js | 117 +++++++++++++----- 4 files changed, 174 insertions(+), 33 deletions(-) diff --git a/packages/builder/lib/processors/minifier.js b/packages/builder/lib/processors/minifier.js index 97242b5f04a..220f40d17c5 100644 --- a/packages/builder/lib/processors/minifier.js +++ b/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); diff --git a/packages/builder/lib/tasks/minify.js b/packages/builder/lib/tasks/minify.js index 4402569d0e5..52a8d4af427 100644 --- a/packages/builder/lib/tasks/minify.js +++ b/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,38 @@ 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 unexpected: the minify task owns the -dbg.js + // path and generates it from the .js. Were both kept, the .js key and the -dbg.js key would + // write the same -dbg.js path, which the shared assertDistinctWrite guard rejects, crashing + // the build. So 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. + // A warning tells the user to exclude both from minification if the pairing is intentional. + 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 sources ` + + `normally must not contain one. If this is intentional (for example the file uses ` + + `ECMAScript syntax the minify task does not support yet), list both ${resourcePath} ` + + `and ${dbgPath} in the "builder.minification.excludes" configuration of the ` + + `project's ui5.yaml.`); + 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 diff --git a/packages/builder/test/lib/tasks/minify.integration.js b/packages/builder/test/lib/tasks/minify.integration.js index 2aa3136d8db..48cd0196b50 100644 --- a/packages/builder/test/lib/tasks/minify.integration.js +++ b/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"); + }); diff --git a/packages/builder/test/lib/tasks/minify.js b/packages/builder/test/lib/tasks/minify.js index e1abdccbe26..cf524dae391 100644 --- a/packages/builder/test/lib/tasks/minify.js +++ b/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,46 @@ 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"); + t.true(log.warn.firstCall.firstArg.includes("builder.minification.excludes"), + "Warning hints at the minification excludes configuration"); + + // 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"); + }); +});