Skip to content

Commit

Permalink
[FIX] Bundler: Ensure reproducibility for bundles with multiple parts (…
Browse files Browse the repository at this point in the history 
…#689)

Sort modules in preload sections alphabetically.
This caused different bundle contents based on the order of modules that
the resolver provides, which is not deterministic.

Remove terser from size estimation as modules are now already optimized
if the 'optimize' bundle option is enabled.
This caused incorrect estimations about the bundle size which resulted
into size differences of the individual parts.
  • Loading branch information
@matz3
matz3 authored Jan 25, 2022
1 parent e749b6a commit 6f4588b
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 58 deletions.
34 changes: 10 additions & 24 deletions lib/lbt/bundle/AutoSplitter.js
View file
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
"use strict";

const terser = require("terser");
const {pd} = require("pretty-data");

const ModuleName = require("../utils/ModuleName");
const {SectionType} = require("./BundleDefinition");
const escapePropertiesFile = require("../utils/escapePropertiesFile");
const log = require("@ui5/logger").getLogger("lbt:bundle:AutoSplitter");

const copyrightCommentsPattern = /copyright|\(c\)(?:[0-9]+|\s+[0-9A-za-z])|released under|license|\u00a9/i;
const xmlHtmlPrePattern = /<(?:\w+:)?pre\b/;

/**
Expand Down Expand Up @@ -112,6 +110,7 @@ class AutoSplitter {

resolvedModule.sections.forEach( (section) => {
let currentSection;
let sequence;
switch ( section.mode ) {
case SectionType.Provided:
// 'provided' sections are no longer needed in a fully resolved module
Expand All @@ -131,16 +130,20 @@ class AutoSplitter {
});
break;
case SectionType.Preload:
sequence = section.modules.slice();
// simple version: just sort alphabetically
sequence.sort();

// NODE_TODO: sort by copyright:
// sequence = section.modules.slice();
// jsBuilder.beforeWriteFunctionPreloadSection((List<ModuleName>) sequence);

currentSection = {
mode: SectionType.Preload,
filters: []
};
currentSection.name = section.name;
currentModule.sections.push( currentSection );
section.modules.forEach( (module) => {
sequence.forEach( (module) => {
const moduleSize = moduleSizes[module];
if ( part + 1 < numberOfParts && totalSize + moduleSize / 2 > partSize ) {
part++;
Expand Down Expand Up @@ -195,26 +198,9 @@ class AutoSplitter {
}

if ( /\.js$/.test(module) ) {
// console.log("determining compressed size for %s", module);
let fileContent = await resource.buffer();
if ( this.optimize ) {
// console.log("uglify %s start", module);
const result = await terser.minify({
[resource.name]: String(fileContent)
}, {
warnings: false, // TODO configure?
compress: false, // TODO configure?
output: {
comments: copyrightCommentsPattern,
wrap_func_args: false
}
// , outFileName: resource.name
// , outSourceMap: true
});
// console.log("uglify %s end", module);
fileContent = result.code;
}
// trace.debug("analyzed %s:%d%n", module, mw.getTargetLength());
// No optimize / minify step here as the input should be
// either already optimized or not, based on the bundle options
const fileContent = await resource.buffer();
return fileContent.length;
} else if ( /\.properties$/.test(module) ) {
/* NODE-TODO minimize *.properties
Expand Down
68 changes: 34 additions & 34 deletions test/lib/lbt/bundle/AutoSplitter.js
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
const test = require("ava");
const sinon = require("sinon");
const terser = require("terser");
const {pd} = require("pretty-data");
const BundleResolver = require("../../../../lib/lbt/bundle/Resolver");
const AutoSplitter = require("../../../../lib/lbt/bundle/AutoSplitter");
Expand Down Expand Up @@ -55,7 +54,7 @@ test("integration: AutoSplitter with numberOfParts 1", async (t) => {
defaultFileTypes: [".js", ".fragment.xml", ".view.xml", ".properties", ".json"],
sections: [{
mode: "preload",
filters: ["a.js", "b.json"],
filters: ["x.view.xml", "c.properties", "b.json"], // intentionally unsorted
resolve: false,
resolveConditional: false,
renderer: false
Expand All @@ -67,7 +66,7 @@ test("integration: AutoSplitter with numberOfParts 1", async (t) => {
name: `Component-preload-0.js`,
sections: [{
mode: "preload",
filters: ["a.js", "b.json"],
filters: ["b.json", "c.properties", "x.view.xml"], // preload section modules should be sorted
name: undefined
}]
});
Expand Down Expand Up @@ -155,11 +154,11 @@ test("_calcMinSize: compressedSize", async (t) => {
};
}
};
const autpSplitter = new AutoSplitter(pool);
t.deepEqual(await autpSplitter._calcMinSize("mymodule.js"), 123);
const autoSplitter = new AutoSplitter(pool);
t.deepEqual(await autoSplitter._calcMinSize("mymodule.js"), 123);
});

test("_calcMinSize: js resource", async (t) => {
test("_calcMinSize: js resource (optimize=false)", async (t) => {
const pool = {
findResourceWithInfo: function() {
return {
Expand All @@ -172,13 +171,11 @@ test("_calcMinSize: js resource", async (t) => {
};
}
};
const autpSplitter = new AutoSplitter(pool);
t.deepEqual(await autpSplitter._calcMinSize("mymodule.js"), 13);
const autoSplitter = new AutoSplitter(pool);
t.deepEqual(await autoSplitter._calcMinSize("mymodule.js"), 13);
});


test.serial("_calcMinSize: uglify js resource", async (t) => {
const stubTerser = sinon.stub(terser, "minify").resolves({code: "123"});
test.serial("_calcMinSize: js resource (optimize=true)", async (t) => {
const pool = {
findResourceWithInfo: function() {
return {
Expand All @@ -191,10 +188,13 @@ test.serial("_calcMinSize: uglify js resource", async (t) => {
};
}
};
const autpSplitter = new AutoSplitter(pool);
autpSplitter.optimize = true;
t.deepEqual(await autpSplitter._calcMinSize("mymodule.js"), 3);
stubTerser.restore();
const autoSplitter = new AutoSplitter(pool);

// The optimize flag should not be taken into account and the resource
// should not get optimized by the AutoSplitter.
autoSplitter.optimize = true;

t.deepEqual(await autoSplitter._calcMinSize("mymodule.js"), 13);
});

test("_calcMinSize: properties resource", async (t) => {
Expand All @@ -221,8 +221,8 @@ test("_calcMinSize: properties resource", async (t) => {
};
}
};
const autpSplitter = new AutoSplitter(pool);
t.deepEqual(await autpSplitter._calcMinSize("mymodule.properties"), 10, "length of 1234\\u00df");
const autoSplitter = new AutoSplitter(pool);
t.deepEqual(await autoSplitter._calcMinSize("mymodule.properties"), 10, "length of 1234\\u00df");
});

test("_calcMinSize: xml view resource", async (t) => {
Expand All @@ -234,9 +234,9 @@ test("_calcMinSize: xml view resource", async (t) => {
};
}
};
const autpSplitter = new AutoSplitter(pool);
autpSplitter.optimizeXMLViews = true;
t.deepEqual(await autpSplitter._calcMinSize("mymodule.view.xml"), 5);
const autoSplitter = new AutoSplitter(pool);
autoSplitter.optimizeXMLViews = true;
t.deepEqual(await autoSplitter._calcMinSize("mymodule.view.xml"), 5);
});

test("_calcMinSize: xml view resource without optimizeXMLViews", async (t) => {
Expand All @@ -248,8 +248,8 @@ test("_calcMinSize: xml view resource without optimizeXMLViews", async (t) => {
};
}
};
const autpSplitter = new AutoSplitter(pool);
t.deepEqual(await autpSplitter._calcMinSize("mymodule.view.xml"), 6);
const autoSplitter = new AutoSplitter(pool);
t.deepEqual(await autoSplitter._calcMinSize("mymodule.view.xml"), 6);
});

test.serial("_calcMinSize: optimize xml view resource", async (t) => {
Expand All @@ -262,10 +262,10 @@ test.serial("_calcMinSize: optimize xml view resource", async (t) => {
};
}
};
const autpSplitter = new AutoSplitter(pool);
autpSplitter.optimizeXMLViews = true;
autpSplitter.optimize = true;
t.deepEqual(await autpSplitter._calcMinSize("mymodule.view.xml"), 6);
const autoSplitter = new AutoSplitter(pool);
autoSplitter.optimizeXMLViews = true;
autoSplitter.optimize = true;
t.deepEqual(await autoSplitter._calcMinSize("mymodule.view.xml"), 6);
stubXmlmin.restore();
});

Expand All @@ -279,11 +279,11 @@ test.serial("_calcMinSize: optimize xml view resource and pre tag", async (t) =>
};
}
};
const autpSplitter = new AutoSplitter(pool);
autpSplitter.optimizeXMLViews = true;
autpSplitter.optimize = true;
const autoSplitter = new AutoSplitter(pool);
autoSplitter.optimizeXMLViews = true;
autoSplitter.optimize = true;
t.false(stubXmlmin.called, "xmlmin should not be called");
t.deepEqual(await autpSplitter._calcMinSize("mymodule.view.xml"), 19);
t.deepEqual(await autoSplitter._calcMinSize("mymodule.view.xml"), 19);
stubXmlmin.restore();
});

Expand All @@ -293,8 +293,8 @@ test("_calcMinSize: no resource", async (t) => {
return null;
}
};
const autpSplitter = new AutoSplitter(pool);
t.deepEqual(await autpSplitter._calcMinSize("mymodule.properties"), 0);
const autoSplitter = new AutoSplitter(pool);
t.deepEqual(await autoSplitter._calcMinSize("mymodule.properties"), 0);
});

test("_calcMinSize: unknown resource with info", async (t) => {
Expand All @@ -307,6 +307,6 @@ test("_calcMinSize: unknown resource with info", async (t) => {
};
}
};
const autpSplitter = new AutoSplitter(pool);
t.deepEqual(await autpSplitter._calcMinSize("mymodule.mjs"), 47);
const autoSplitter = new AutoSplitter(pool);
t.deepEqual(await autoSplitter._calcMinSize("mymodule.mjs"), 47);
});

0 comments on commit 6f4588b

Please sign in to comment.