[FEATURE] Minifier: Support input source maps

[RandomByte]

If a resource references a source map (for example from an earlier transpilation):

1. Use that source map for the debug variant of the resource
2. Pass it to terser so it can update it based on the transformation, preserving the mapping to the initial source

Before this change, the minifier processor always created a new source map based on the minification of the resource. This change is active by default. However, if a resource is changed before the minify task is executed (for example by tasks like replaceCopyright), we assume that the referenced source map is potentially corrupt and therefore ignore it.

The general logic is very similar to what we are already doing in lbt/bundle/Builder.js.

Documentation: https://sap.github.io/ui5-tooling/stable/pages/Builder/#minify

Open points (also see comments below):

• Should we use existing source maps by default?
  • If resources have been modified in the meantime (for example by preceding tasks such as replaceVersion or replaceCopyright), the corresponding source map is likely invalid and should not be used. It's better to have no map than an incorrect one
  • Until we find a solution to update source maps in tasks modifying resource content, we should probably make this opt-in. Maybe even limiting it to resources matching a provided glob pattern
  • ✅ Result: We agreed to check whether a resource has been modified during the UI5 Tooling build. If that is the case, we assume that the corresponding source map is potentially corrupt and ignore it.

JIRA: CPOUI5FOUNDATION-467

[coveralls]

coverage: 94.408% (-0.02%) from 94.427% when pulling 390fb81 on minifier-input-sourcemap into 30c4c14 on main.

[petermuessig]

@RandomByte instead of just relying on inline source maps, I would extend to logic a bit more and depend on the sourceMappingURL information, e.g. lib/processors/minifier.js:

import {fileURLToPath} from "node:url";
import posixPath from "node:path/posix";
import os from "node:os";
import workerpool from "workerpool";
import Resource from "@ui5/fs/Resource";
import {getLogger} from "@ui5/logger";
const log = getLogger("builder:processors:minifier");

const debugFileRegex = /((?:\.view|\.fragment|\.controller|\.designtime|\.support)?\.js(?:\.map)?)$/;

const MIN_WORKERS = 2;
const MAX_WORKERS = 4;
const osCpus = os.cpus().length || 1;
const maxWorkers = Math.max(Math.min(osCpus - 1, MAX_WORKERS), MIN_WORKERS);

// Shared workerpool across all executions until the taskUtil cleanup is triggered
let pool;

function getPool(taskUtil) {
	if (!pool) {
		log.verbose(`Creating workerpool with up to ${maxWorkers} workers (available CPU cores: ${osCpus})`);
		const workerPath = fileURLToPath(new URL("./minifierWorker.js", import.meta.url));
		pool = workerpool.pool(workerPath, {
			workerType: "auto",
			maxWorkers
		});
		taskUtil.registerCleanupTask(() => {
			log.verbose(`Terminating workerpool`);
			const poolToBeTerminated = pool;
			pool = null;
			poolToBeTerminated.terminate();
		});
	}
	return pool;
}

async function minifyInWorker(options, taskUtil) {
	return getPool(taskUtil).exec("execMinification", [options]);
}

/**
 * @public
 * @module @ui5/builder/processors/minifier
 */

/**
 * Result set
 *
 * @public
 * @typedef {object} MinifierResult
 * @property {@ui5/fs/Resource} resource Minified resource
 * @property {@ui5/fs/Resource} dbgResource Debug (non-minified) variant
 * @property {@ui5/fs/Resource} sourceMap Source Map
 */

/**
 * Minifies the supplied resources.
 *
 * @public
 * @function default
 * @static
 *
 * @param {object} parameters Parameters
 * @param {@ui5/fs/Resource[]} parameters.resources List of resources to be processed
 * @param {@ui5/builder/tasks/TaskUtil|object} [parameters.taskUtil] TaskUtil instance.
 *    Required when using the <code>useWorkers</code> option
 * @param {object} [parameters.options] Options
 * @param {boolean} [parameters.options.addSourceMappingUrl=true]
 *   Whether to add a sourceMappingURL reference to the end of the minified resource
 * @param {boolean} [parameters.options.useWorkers=false]
 *  Whether to offload the minification task onto separate CPU threads. This often speeds up the build process
 * @returns {Promise<module:@ui5/builder/processors/minifier~MinifierResult[]>}
 *   Promise resolving with object of resource, dbgResource and sourceMap
 */
export default async function({resources, taskUtil, options: {addSourceMappingUrl = true, useWorkers = false} = {}, workspace}) {
	let minify;
	if (useWorkers) {
		if (!taskUtil) {
			// TaskUtil is required for worker support
			throw new Error(`Minifier: Option 'useWorkers' requires a taskUtil instance to be provided`);
		}
		minify = minifyInWorker;
	} else {
		// Do not use workerpool
		minify = (await import("./minifierWorker.js")).default;
	}

	return Promise.all(resources.map(async (resource) => {
		const dbgPath = resource.getPath().replace(debugFileRegex, "-dbg$1");
		const dbgResource = await resource.clone();
		dbgResource.setPath(dbgPath);

		const filename = posixPath.basename(resource.getPath());
		const code = await resource.getString();

		const sourceMapOptions = {
			filename
		};
		if (addSourceMappingUrl) {
			sourceMapOptions.url = filename + ".map";
		}

		const dbgFilename = posixPath.basename(dbgPath);

		// find existing source map (either from sourceMappingURL or by naming convention)
		const sourceMappingURL = code.match(/^\/\/# sourceMappingURL=(.*)$/m)?.[1] || filename + ".map";
		if (sourceMappingURL.startsWith("data:")) {
			sourceMapOptions.content = "inline";
		} else {
			const sourceMap = await workspace.byPath(posixPath.resolve(posixPath.dirname(resource.getPath()), sourceMappingURL));
			if (sourceMap) {
				sourceMapOptions.content = await sourceMap.getString();
				// create the -dbg source map
				const dbgSourceMapJSON = JSON.parse(sourceMapOptions.content);
				dbgSourceMapJSON.file = dbgFilename;
				const dbgSourceMapPath = sourceMap.getPath().replace(debugFileRegex, "-dbg$1");
				const dbgSourceMapResource = new Resource({
					path: dbgSourceMapPath,
					string: JSON.stringify(dbgSourceMapJSON)
				});
				await workspace.write(dbgSourceMapResource);
			}
		}

		const result = await minify({
			filename,
			dbgFilename,
			code,
			sourceMapOptions
		}, taskUtil);
		resource.setString(result.code);
		const sourceMapResource = new Resource({
			path: resource.getPath() + ".map",
			string: result.map
		});
		return {resource, dbgResource, sourceMapResource};
	}));
}

I used this to verify the source maps handling in the ui5-ecosystem-showcase project by patching the ui5-builder with the above code (it also requires to pass the workspace so far to read the file defined in the sourceMappingURL. It works with inline source maps, with externals and it also creates the source maps for the -dbg files which maybe also are based on TypeScript or transpiled. Maybe we can chat about this soon...

[RandomByte]

Thanks Peter. Looks good! Maybe we can also reuse the code from our bundle/Builder module

The main question for me is whether we should always search for source maps in the code or rather make this a ui5.yaml configuration option. For this we should benchmark the performance impact of always doing it.

[petermuessig]

How expensive is the workspace.byPath operation? Is this only a lookup in a map? If yes, I don't see much overhead to do so. You anyhow need to read it in any case if there is a sourceMappingURL defined to ensure that it is properly considered. As said, I would always run this logic if the byPath function works effectively.

[petermuessig]

@RandomByte @matz3 any chance to fix this before the vacation starts? 😉

[RandomByte]

Some things we need to consider when using exiting source maps:

Existing source maps for resources that are modified by tasks preceding the minify tasks are likely to become inconsistent. For example if a resource is transpiled from TypeScript to JavaScript right at the beginning of the build, the replaceCopyright task might insert a multi-line copyright statement at the beginning of the JavaScript resource, thus invalidating the source map.

This is not an issue for current TypeScript scenarios where typically ui5-tooling-transpile is being used and configured to run afterTask: replaceVersion. However, those projects then can't use the standard replaceCopyright or replaceVersion tasks and need to find other solutions for those.

To solve this aspect we would probably need to enhance the Resource class to maintain relationships between resources and their source map resources. And we need a central utility module to easily align source maps with the changes done by a task. Rollup and other bundlers have similar functionality, so it makes sense to integrate this into UI5 Tooling at some point.

[petermuessig]

Any chance for a release? 😄

[petermuessig]

Cc @matz3 @flovogt @d3xter666

[matz3]

We decided to remove this path, as it's not expected to have a Source Map file without a reference in the JavaScript file. Furthermore, if this is the case, it might be on purpose so that the existing Source Map is not used as input Source Map.

[codeworrior]

For the records: according to the source map specs, a server can send an HTTP header to provide a reference to the source map for a resource it delivers. This is the only case where a source map would be needed without a reference in the code.

In the context of UI5, however, we're not aware of any scenario that makes use of this feature and therefore currently don't consider it as a use case for the tooling.

[RandomByte]

Some benchmarks according to our documentation, comparing this PR to the current main branch:

openui5-sample-app

ui5-builder Ref	Command	Mean [ms]	Min [ms]	Max [ms]	Relative
main (e96a2be)	ui5 build	730.3 ± 25.7	699.0	786.5	Baseline
minifier-input-sourcemap (b50869d	ui5 build	714.3 ± 9.1	703.5	728.4	-2% (basically the same)

sap.ui.core

ui5-builder Ref	Command	Mean [s]	Min [s]	Max [s]	Relative
main (e96a2be)	ui5 build	8.757 ± 0.167	8.496	9.061	Baseline
minifier-input-sourcemap (b50869d	ui5 build	8.600 ± 0.151	8.325	8.745	-1.7% (basically the same)

OpenUI5 testsuite (including dependencies)

ui5-builder Ref	Command	Mean [s]	Min [s]	Max [s]	Relative
main (e96a2be)	ui5 build --all	58.487 ± 0.541	57.440	59.186	Baseline
minifier-input-sourcemap (b50869d	ui5 build --all	58.622 ± 1.507	56.833	60.867	-0.8% (the same!)

Internal TS Library

ui5-builder Ref	Command	Mean [s]	Min [s]	Max [s]	Relative
main (e96a2be)	ui5 build	5.528 ± 0.228	5.348	5.985	Baseline
minifier-input-sourcemap (b50869d	ui5 build	5.520 ± 0.227	5.364	5.988	= The same =

Specs: MacBook Pro M1 Max

[matz3]

parameter description incomplete

[RandomByte]

Done

[petermuessig]

What's the status on this one here?

[matz3]

What's the status on this one here?

We've clarified all open points and are working on finalising this PR in the upcoming sprint.

[RandomByte]

ui5-fs Resource change to ensure we don't use source maps of resources that have been modified by previous tasks, even when they exist in-memory only: SAP/ui5-fs#524

[RandomByte]

Verified successful with all planned scenarios: easy-ui5 ts-app, easy-ui5 ts-library and an internal TS library.

This PR is ready from my side and only requires final review 👍

[KlattG]

Just some minor stuff - hope I understood it correctly.

outdated

[matz3]

Tests for minify task should be enhanced

[RandomByte]

Tests for minify task should be enhanced

Done