[INTERNAL] generateLibraryPreload: set requiresTopLevelScope correctly (

#508) Ensures that requiresTopLevelScope option in raw-module configuration from .library is preferred and used when creating the library preload. Builder.js: Use correct variable for ModuleInfo (exposedGlobals) to generate the correct globals assignment in library-preload.js Co-authored-by: Frank Weigel <[email protected]>
SAP · Sep 1, 2020 · 03b1aef · 03b1aef
1 parent 9375199
commit 03b1aef
Show file tree

Hide file tree

Showing 14 changed files with 197 additions and 42 deletions.
diff --git a/lib/lbt/bundle/Builder.js b/lib/lbt/bundle/Builder.js
@@ -381,6 +381,14 @@ class BundleBuilder {
 	beforeWritePreloadModule(module, info, resource) {
 	}
 
+	/**
+	 *
+	 * @param {string} module module name
+	 * @param {ModuleInfo} info
+	 * @param {module:@ui5/fs.Resource} resource
+	 * @param {boolean} avoidLazyParsing
+	 * @returns {Promise<boolean>}
+	 */
 	async writePreloadModule(module, info, resource, avoidLazyParsing) {
 		const outW = this.outW;
 
@@ -440,12 +448,18 @@ class BundleBuilder {
 		return true;
 	}
 
+	/**
+	 * Create exports for globals
+	 *
+	 * @param {ModuleInfo} info
+	 */
 	exportGlobalNames(info) {
-		if ( !info || !info.exposedGlobalNames || !info.exposedGlobalNames.length ) {
+		if ( !info || !info.exposedGlobals || !info.exposedGlobals.length ) {
 			return;
 		}
 		this.outW.ensureNewLine();
-		info.exposedGlobalNames.forEach( (globalName) => {
+		info.exposedGlobals.forEach( (globalName) => {
+			// Note: globalName can be assumed to be a valid identifier as it is used as variable name anyhow
 			this.outW.writeln(`this.${globalName}=${globalName};`);
 		});
 	}

diff --git a/lib/lbt/resources/LibraryFileAnalyzer.js b/lib/lbt/resources/LibraryFileAnalyzer.js
@@ -5,7 +5,7 @@
 "use strict";
 
 const xml2js = require("xml2js");
-const ModuleInfo = require("./ModuleInfo");
+const log = require("@ui5/logger").getLogger("lbt:resources:LibraryFileAnalyzer");
 
 const parser = new xml2js.Parser({
 	// explicitChildren: true,
@@ -20,29 +20,38 @@ function getAttribute(node, attr) {
 	return (node.$ && node.$[attr] && node.$[attr].value) || null;
 }
 
-function makeModuleInfo(rawModule) {
+/*
+ * Analyzes the given XML2JS object `rawModule` and creates a rawInfo object from it.
+ * @param {object} rawModule XML2JS object, representing a &lt;raw-module&gt; node from a .library file
+ * @returns {{name:string,dependencies?:string[],requiresTopLevelScope?:boolean,ignoredGlobals?:string[]}
+ */
+function createRawInfo(rawModule) {
 	const name = getAttribute(rawModule, "name");
-	const deps = getAttribute(rawModule, "depends");
 	if ( name ) {
-		const info = new ModuleInfo(name);
+		const rawInfo = {
+			name,
+			rawModule: true,
+			dependencies: []
+		};
+		const deps = getAttribute(rawModule, "depends");
 		if ( deps != null ) {
-			deps.trim().split(/\s*,\s*/).forEach( (dep) => info.addDependency(dep) );
+			rawInfo.dependencies = deps.trim().split(/\s*,\s*/);
+		}
+		const requiresTopLevelScope = getAttribute(rawModule, "requiresTopLevelScope");
+		if ( requiresTopLevelScope ) {
+			rawInfo.requiresTopLevelScope = requiresTopLevelScope === "true";
 		}
-		info.rawModule = true;
-		info.requiresTopLevelScope = getAttribute(rawModule, "requiresTopLevelScope") === "true";
 		const ignoredGlobals = getAttribute(rawModule, "ignoredGlobals");
 		if ( ignoredGlobals ) {
-			info.ignoredGlobals = ignoredGlobals.trim().split(/\s*,\s*/);
+			rawInfo.ignoredGlobals = ignoredGlobals.trim().split(/\s*,\s*/);
 		}
-		// console.log(info);
-		return info;
+		return rawInfo;
 	}
 }
 
-function getDependencyInfos( content ) {
+function getDependencyInfos( name, content ) {
 	const infos = {};
 	parser.parseString(content, (err, result) => {
-		// console.log(JSON.stringify(result, null, '\t'));
 		if ( result &&
 				result.library &&
 				Array.isArray(result.library.appData) &&
@@ -54,9 +63,10 @@ function getDependencyInfos( content ) {
 						Array.isArray(packaging["module-infos"]) ) {
 					packaging["module-infos"].forEach( function(moduleInfos) {
 						moduleInfos["raw-module"] && moduleInfos["raw-module"].forEach( (rawModule) => {
-							const info = makeModuleInfo(rawModule);
-							if ( info ) {
-								infos[info.name] = info;
+							const rawInfo = createRawInfo(rawModule);
+							if ( rawInfo ) {
+								log.verbose(name + " rawInfo:", JSON.stringify(rawInfo));
+								infos[rawInfo.name] = rawInfo;
 							}
 						});
 					});

diff --git a/lib/lbt/resources/ModuleInfo.js b/lib/lbt/resources/ModuleInfo.js
@@ -202,6 +202,18 @@ class ModuleInfo {
 		return Object.keys(this._dependencies);
 	}
 
+	/**
+	 * Removes the given set of `ignoredGlobals` from the set of exposed global names.
+	 *
+	 * @param {string[]} ignoredGlobals Names to ignore (determined from shims in .library)
+	 */
+	removeIgnoredGlobalNames(ignoredGlobals) {
+		if ( this.exposedGlobals ) {
+			const remaining = this.exposedGlobals.filter((global) => !ignoredGlobals.includes(global));
+			this.exposedGlobals = remaining.length > 0 ? remaining : null;
+		}
+	}
+
 	toString() {
 		return "ModuleInfo(" +
 			this.name +
@@ -236,13 +248,6 @@ public class ModuleInfo {
 	 *
 	private boolean excludeFromAllInOne;
 
-
-	public void removeIgnoredGlobalNames(Collection<String> ignoredNames) {
-		if ( !exposedGlobals.isEmpty() ) {
-			exposedGlobals.removeAll(ignoredNames);
-		}
-	}
-
 } */
 
 module.exports = ModuleInfo;
diff --git a/lib/lbt/resources/ResourcePool.js b/lib/lbt/resources/ResourcePool.js
@@ -75,12 +75,17 @@ async function determineDependencyInfo(resource, rawInfo, pool) {
 		if ( rawInfo ) {
 			info.rawModule = true;
 			// console.log("adding preconfigured dependencies for %s:", resource.name, rawInfo.dependencies);
-			rawInfo.dependencies.forEach( (dep) => info.addDependency(dep) );
-			if ( rawInfo.requiresTopLevelScope ) {
-				info.requiresTopLevelScope = true;
+			if ( rawInfo.dependencies ) {
+				rawInfo.dependencies.forEach( (dep) => info.addDependency(dep) );
 			}
+
+			if ( rawInfo.requiresTopLevelScope != null ) {
+				// an explicitly defined value for requiresTopLevelScope from .library overrides analysis result
+				info.requiresTopLevelScope = rawInfo.requiresTopLevelScope;
+			}
+
 			if ( rawInfo.ignoredGlobals ) {
-				info.ignoredGlobals = rawInfo.ignoredGlobals;
+				info.removeIgnoredGlobalNames(rawInfo.ignoredGlobals);
 			}
 		}
 		if ( /(?:^|\/)Component\.js/.test(resource.name) ) {
@@ -153,7 +158,7 @@ class ResourcePool {
 		if ( /\.library$/.test(resource.name) ) {
 			// read raw-module info from .library files
 			return resource.buffer().then( (buffer) => {
-				const infos = LibraryFileAnalyzer.getDependencyInfos( buffer );
+				const infos = LibraryFileAnalyzer.getDependencyInfos( resource.name, buffer );
 				for ( const name of Object.keys(infos) ) {
 					this._rawModuleInfos.set(name, infos[name]);
 				}

diff --git a/test/expected/build/sap.ui.core/preload/resources/sap/ui/core/.library b/test/expected/build/sap.ui.core/preload/resources/sap/ui/core/.library
@@ -8,4 +8,13 @@
 
 	<documentation>Core</documentation>
 
+	<appData>
+		<packaging xmlns="http://www.sap.com/ui5/buildext/packaging" version="2.0" >
+			  <module-infos>
+				<raw-module name="sap/ui/core/one.js"
+					requiresTopLevelScope="false" />
+			</module-infos>
+		</packaging>
+	</appData>
+
 </library>
diff --git a/test/expected/build/sap.ui.core/preload/resources/sap/ui/core/library-preload.js b/test/expected/build/sap.ui.core/preload/resources/sap/ui/core/library-preload.js
@@ -2,6 +2,9 @@
 jQuery.sap.registerPreloadedModules({
 "version":"2.0",
 "modules":{
+	"sap/ui/core/one.js":function(){function One(){return 1}
+this.One=One;
+},
 	"sap/ui/core/some.js":function(){/*!
  * ${copyright}
  */

diff --git a/test/expected/build/sap.ui.core/preload/resources/sap/ui/core/one.js b/test/expected/build/sap.ui.core/preload/resources/sap/ui/core/one.js
@@ -0,0 +1,3 @@
+function One(){
+	return 1;
+}
diff --git a/test/fixtures/sap.ui.core/main/src/sap/ui/core/.library b/test/fixtures/sap.ui.core/main/src/sap/ui/core/.library
@@ -8,4 +8,13 @@
 
 	<documentation>Core</documentation>
 
+	<appData>
+		<packaging xmlns="http://www.sap.com/ui5/buildext/packaging" version="2.0" >
+			  <module-infos>
+				<raw-module name="sap/ui/core/one.js"
+					requiresTopLevelScope="false" />
+			</module-infos>
+		</packaging>
+	</appData>
+
 </library>
diff --git a/test/fixtures/sap.ui.core/main/src/sap/ui/core/one.js b/test/fixtures/sap.ui.core/main/src/sap/ui/core/one.js
@@ -0,0 +1,3 @@
+function One(){
+	return 1;
+}
diff --git a/test/lib/lbt/bundle/Builder.js b/test/lib/lbt/bundle/Builder.js
@@ -46,6 +46,64 @@ test.serial("writePreloadModule: with invalid json content", async (t) => {
 	t.is(writeStub.callCount, 1, "Writer is called once");
 });
 
+test("integration: createBundle with exposedGlobals", async (t) => {
+	const pool = new ResourcePool();
+	pool.addResource({
+		name: "a.js",
+		buffer: async () => "function One(){return 1;}"
+	});
+	pool.addResource({
+		name: "ui5loader.js",
+		buffer: async () => ""
+	});
+	pool.addResource({
+		name: "a.library",
+		buffer: async () => `<?xml version="1.0" encoding="UTF-8" ?>
+<library xmlns="http://www.sap.com/sap.ui.library.xsd" >
+	<appData>
+		<packaging xmlns="http://www.sap.com/ui5/buildext/packaging" version="2.0" >
+			  <module-infos>
+				<raw-module name="a.js"
+					requiresTopLevelScope="false" />
+			</module-infos>
+		</packaging>
+	</appData>
+</library>`
+	});
+
+	const bundleDefinition = {
+		name: `library-preload.js`,
+		defaultFileTypes: [".js"],
+		sections: [{
+			mode: "preload",
+			name: "preload-section",
+			filters: ["a.js"]
+		}, {
+			mode: "require",
+			filters: ["ui5loader.js"]
+		}]
+	};
+
+	const builder = new Builder(pool);
+	const oResult = await builder.createBundle(bundleDefinition, {numberOfParts: 1, decorateBootstrapModule: true});
+	t.deepEqual(oResult.name, "library-preload.js");
+	const expectedContent = `//@ui5-bundle library-preload.js
+sap.ui.require.preload({
+	"a.js":function(){function One(){return 1;}
+this.One=One;
+}
+},"preload-section");
+sap.ui.requireSync("ui5loader");
+`;
+	t.deepEqual(oResult.content, expectedContent, "EVOBundleFormat " +
+		"should contain:" +
+		" preload part from a.js" +
+		" require part from ui5loader.js");
+	t.deepEqual(oResult.bundleInfo.name, "library-preload.js", "bundle info name is correct");
+	t.deepEqual(oResult.bundleInfo.size, expectedContent.length, "bundle info size is correct");
+	t.deepEqual(oResult.bundleInfo.subModules, ["a.js"],
+		"bundle info subModules are correct");
+});
 
 test("integration: createBundle EVOBundleFormat (ui5loader.js)", async (t) => {
 	const pool = new ResourcePool();

diff --git a/test/lib/lbt/resources/LibraryFileAnalyzer.js b/test/lib/lbt/resources/LibraryFileAnalyzer.js
@@ -48,7 +48,7 @@ test("extract packaging info from .library file", (t) => {
 		}
 	];
 
-	const actual = LibraryFileAnalyzer.getDependencyInfos(libraryFile);
+	const actual = LibraryFileAnalyzer.getDependencyInfos("a.library", libraryFile);
 
 	t.deepEqual(Object.keys(actual), expectedInfos.map((exp) => exp.name),
 		"Method should return the expected set of modules");

diff --git a/test/lib/lbt/resources/ModuleInfo.js b/test/lib/lbt/resources/ModuleInfo.js
@@ -61,3 +61,14 @@ test("ModuleInfo: toString", async (t) => {
 	// expectation
 	t.is(stringContent, "ModuleInfo(myName, dependencies=dep1,dep2, includes=sub1,sub2)", "string value is correct");
 });
+
+test("ModuleInfo: removeIgnoredGlobalNames", (t) => {
+	// setup
+	const moduleInfo = new ModuleInfo("myName");
+	moduleInfo.exposedGlobals = ["supi", "dupi"];
+
+	moduleInfo.removeIgnoredGlobalNames(["hop", "supi"]);
+
+	// expectation
+	t.deepEqual(moduleInfo.exposedGlobals, ["dupi"], "exposedGlobals are correct");
+});
diff --git a/test/lib/lbt/resources/ResourcePool.js b/test/lib/lbt/resources/ResourcePool.js
@@ -113,6 +113,32 @@ test("getModuleInfo", async (t) => {
 	t.deepEqual(jsResource.subModules, [], "does not contain submodules");
 });
 
+test("getModuleInfo: determineDependencyInfo for raw js resources", async (t) => {
+	const resourcePool = new ResourcePool();
+	const code = `function One() {return 1;}`;
+	const inputJsResource = {name: "a.js", buffer: async () => code};
+	resourcePool.addResource(inputJsResource);
+
+
+	const infoA = new ModuleInfo("a.js");
+	infoA.requiresTopLevelScope = false;
+
+	const stubGetDependencyInfos = sinon.stub(LibraryFileAnalyzer, "getDependencyInfos").returns({
+		"a.js": infoA
+	});
+
+	const library = {
+		name: "a.library",
+		buffer: async () => ""
+	};
+	await resourcePool.addResource(library);
+
+	const jsResource = await resourcePool.getModuleInfo("a.js");
+	t.false(jsResource.requiresTopLevelScope);
+
+	stubGetDependencyInfos.restore();
+});
+
 test("getModuleInfo: determineDependencyInfo for js templateAssembler code", async (t) => {
 	const resourcePool = new ResourcePool();
 	const code = `sap.ui.define(["a", "sap/fe/core/TemplateAssembler"], function(a, TemplateAssembler){
@@ -218,13 +244,15 @@ test("addResource twice", async (t) => {
 test.serial("addResource: library and eval raw module info", async (t) => {
 	const resourcePool = new ResourcePool();
 
-	const infoA = new ModuleInfo("moduleA.js");
+	const infoA = {};
+	infoA.name = "moduleA.js";
 	infoA.rawModule = true;
-	infoA.addDependency("123.js");
+	infoA.dependencies = ["123.js"];
 	infoA.ignoredGlobals = ["foo", "bar"];
-	const infoB = new ModuleInfo("moduleB.js");
+	const infoB = {};
+	infoB.name = "moduleB.js";
 	infoB.rawModule = true;
-	infoB.addDependency("456.js");
+	infoB.dependencies = ["456.js"];
 
 	const stubGetDependencyInfos = sinon.stub(LibraryFileAnalyzer, "getDependencyInfos").returns({
 		"moduleA.js": infoA,
@@ -233,7 +261,7 @@ test.serial("addResource: library and eval raw module info", async (t) => {
 
 	const library = {
 		name: "a.library",
-		buffer: async () => ""
+		buffer: async () => "" // LibraryFileAnalyzer.getDependencyInfos() is stubbed! Therefore this content is irrelevant.
 	};
 	await resourcePool.addResource(library);
 	const moduleA = {
@@ -258,11 +286,9 @@ test.serial("addResource: library and eval raw module info", async (t) => {
 	t.true(actualResourceA.info instanceof ModuleInfo);
 	t.deepEqual(actualResourceA.info.dependencies, ["123.js"],
 		"configured dependencies should have been dded");
-	t.true(actualResourceA.info.requiresTopLevelScope);
-	t.deepEqual(actualResourceA.info.exposedGlobals, ["foo", "bar", "some"],
-		"global names should be known from analsyis step");
-	t.deepEqual(actualResourceA.info.ignoredGlobals, ["foo", "bar"],
-		"ignored globals should have been taken from .library");
+	t.true(actualResourceA.info.requiresTopLevelScope, "'some' is the global variable to be exposed");
+	t.deepEqual(actualResourceA.info.exposedGlobals, ["some"],
+		"global names should be known from analysis step");
 
 	const actualResourceB = await resourcePool.findResourceWithInfo("moduleB.js");
 	t.true(actualResourceB.info instanceof ModuleInfo);
@@ -271,7 +297,6 @@ test.serial("addResource: library and eval raw module info", async (t) => {
 	t.true(actualResourceB.info.requiresTopLevelScope);
 	t.deepEqual(actualResourceB.info.exposedGlobals, ["foo", "bar", "some"],
 		"global names should be known from analsyis step");
-	t.deepEqual(actualResourceB.info.ignoredGlobals, undefined);
 
 	stubGetDependencyInfos.restore();
 });