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..47e0314f7a6 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,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 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..02b565d5fdc 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,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"); + }); +});