Skip to content

[FIX] generateResourcesJson: Make resources.json generation deterministic #551

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 30, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 36 additions & 15 deletions lib/lbt/resources/ResourceCollector.js
Original file line number Diff line number Diff line change
Expand Up @@ -150,16 +150,25 @@ class ResourceCollector {
});
}

async determineResourceDetails({pool, debugResources, mergedResources, designtimeResources, supportResources}) {
async determineResourceDetails({
debugResources, mergedResources, designtimeResources, supportResources, debugBundles
}) {
const baseNames = new Set();
const debugFilter = new ResourceFilterList(debugResources);
const mergeFilter = new ResourceFilterList(mergedResources);
const designtimeFilter = new ResourceFilterList(designtimeResources);
const supportFilter = new ResourceFilterList(supportResources);
const debugBundleFilter = new ResourceFilterList(debugBundles);

const promises = [];
const nonBundledDebugResources = [];

for (const [name, info] of this._resources.entries()) {
if ( debugFilter.matches(name) ) {
info.isDebug = true;
log.verbose(` found potential debug resource '${name}'`);
}

// log.verbose(` checking ${name}`);
let m;
if ( m = LOCALE.exec(name) ) {
Expand All @@ -180,9 +189,14 @@ class ResourceCollector {
}

if ( /(?:\.js|\.view\.xml|\.control\.xml|\.fragment\.xml)$/.test(name) ) {
promises.push(
this.enrichWithDependencyInfo(info)
);
if ( (!info.isDebug || debugBundleFilter.matches(name)) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this strange now to focus on non-debug resources here, but then in the ResourcePool doing the opposite and prefer the debug resource?

My statement that we should run over the non-debug sources was motivated by the fact that not every resources has a related dbg-resource.

But couldn't we check that here in the ResourceCollector and keep the logic out of the pool?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, the mentioned change in ResourcePool has been retracted from this PR. Just to have it documented somewhere I created #553

// Only analyze non-debug files and special debug bundles (like sap-ui-core-dbg.js)
promises.push(
this.enrichWithDependencyInfo(info)
);
} else {
nonBundledDebugResources.push(info);
}
}

// set the module name for .properties and .json
Expand All @@ -197,11 +211,6 @@ class ResourceCollector {
}));
}

if ( debugFilter.matches(name) ) {
info.isDebug = true;
log.verbose(` found potential debug resource '${name}'`);
}

if ( mergeFilter.matches(name) ) {
info.merged = true;
log.verbose(` found potential merged resource '${name}'`);
Expand All @@ -226,7 +235,21 @@ class ResourceCollector {
}
}

return Promise.all(promises);
await Promise.all(promises);

for (let i = nonBundledDebugResources.length - 1; i >= 0; i--) {
const dbgInfo = nonBundledDebugResources[i];
const nonDebugName = ResourceInfoList.getNonDebugName(dbgInfo.name);
const nonDbgInfo = this._resources.get(nonDebugName);
const newDbgInfo = new ResourceInfo(dbgInfo.name);

// First copy info of analysis from non-dbg file (included, required, condRequired, ...)
newDbgInfo.copyFrom(null, nonDbgInfo);
// Then copy over info from dbg file to properly set name, isDebug, etc.
newDbgInfo.copyFrom(null, dbgInfo);

this._resources.set(dbgInfo.name, newDbgInfo);
}
}

createOrphanFilters() {
Expand All @@ -251,21 +274,19 @@ class ResourceCollector {
return filtersByComponent;
}

groupResourcesByComponents(options) {
groupResourcesByComponents() {
const orphanFilters = this.createOrphanFilters();
const debugBundlesFilter = new ResourceFilterList(options.debugBundles);
for (const resource of this._resources.values()) {
let contained = false;
for (const [prefix, list] of this._components.entries()) {
const isDebugBundle = debugBundlesFilter.matches(resource.name);
if ( resource.name.startsWith(prefix) ) {
list.add(resource, !isDebugBundle);
list.add(resource);
contained = true;
} else if ( orphanFilters.has(prefix) ) {
// log.verbose(` checking '${resource.name}' against orphan filter ` +
// `'${orphanFilters.get(prefix)}' (${prefix})`);
if ( orphanFilters.get(prefix).matches(resource.name) ) {
list.add(resource, !isDebugBundle);
list.add(resource);
contained = true;
}
}
Expand Down
37 changes: 1 addition & 36 deletions lib/lbt/resources/ResourceInfoList.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
const ResourceInfo = require("./ResourceInfo");

const DEBUG_RESOURCES_PATTERN = /-dbg((?:\.view|\.fragment|\.controller|\.designtime|\.support)?\.js|\.css)$/;
const RESOURCES_PATTERN = /((?:\.view|\.fragment|\.controller|\.designtime|\.support)?\.js|\.css)$/;

/**
* A list of ResourceInfo objects, suitable for (but not dependent on) JSON serialization.
Expand Down Expand Up @@ -39,37 +38,12 @@ class ResourceInfoList {
* Add ResourceInfo to list
*
* @param {ResourceInfo} info
* @param {boolean} shareDebugInformation
*/
add(info, shareDebugInformation=true) {
add(info) {
const relativeName = ResourceInfoList.makePathRelativeTo(this.name, info.name);

// search for a resource with the same name
let myInfo = this.resourcesByName.get(relativeName);

if ( myInfo == null && shareDebugInformation) {
// when not found, check if the given resource is a debug resource and
// share the information with the non-dbg version
const nonDbgName = ResourceInfoList.getNonDebugName(relativeName);
const dbgName = ResourceInfoList.getDebugName(relativeName);
if ( nonDbgName != null && this.resourcesByName.has(nonDbgName) ) {
// copy from source
myInfo = new ResourceInfo(relativeName);
const source = this.resourcesByName.get(nonDbgName);
myInfo.copyFrom(this.name, source);
this.resources.push(myInfo);
this.resourcesByName.set(relativeName, myInfo);
} else if (dbgName != null && this.resourcesByName.has(dbgName)) {
// copy from debug
myInfo = new ResourceInfo(relativeName);
const source = this.resourcesByName.get(dbgName);
myInfo.copyFrom(this.name, source);
myInfo.module = ResourceInfoList.getNonDebugName(source.module);
this.resources.push(myInfo);
this.resourcesByName.set(relativeName, myInfo);
}
}

// this is the assumption, that the debug one is the same as the non-dbg one
if ( myInfo == null ) {
myInfo = new ResourceInfo(relativeName);
Expand Down Expand Up @@ -139,15 +113,6 @@ class ResourceInfoList {
}
return null;
}

static getDebugName(path) {
if ( RESOURCES_PATTERN.test(path) ) {
if (!path.replace(RESOURCES_PATTERN, "").endsWith("-dbg")) {
return path.replace( RESOURCES_PATTERN, "-dbg$1");
}
}
return null;
}
}

module.exports = ResourceInfoList;
10 changes: 5 additions & 5 deletions lib/processors/resourceListCreator.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ const DEFAULT_SUPPORT_RESOURCES_FILTER = [
* @type {string[]}
*/
const DEBUG_BUNDLES = [
"sap-ui-core-dbg.js"
"sap-ui-core-dbg.js",
"sap-ui-core-nojQuery-dbg.js"
];

/**
Expand Down Expand Up @@ -159,13 +160,12 @@ module.exports = async function({resources, options}) {
debugResources: options.debugResources,
mergedResources: options.mergedResources,
designtimeResources: options.designtimeResources,
supportResources: options.supportResources
supportResources: options.supportResources,
debugBundles: options.debugBundles
});

// group resources by components and create ResourceInfoLists
collector.groupResourcesByComponents({
debugBundles: options.debugBundles
});
collector.groupResourcesByComponents();

const resourceLists = [];

Expand Down
80 changes: 70 additions & 10 deletions test/lib/lbt/resources/ResourceCollector.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ test.serial("groupResourcesByComponents: debugBundles", async (t) => {
});
await resourceCollector.visitResource({getPath: () => "/resources/testcomp/Component.js", getSize: async () => 13});
await resourceCollector.visitResource({getPath: () => "/resources/my/file.js", getSize: async () => 13});
resourceCollector.groupResourcesByComponents({debugBundles: [".*-dbg.js"]});
resourceCollector.groupResourcesByComponents();
t.is(resourceCollector.resources.size, 0, "all resources were deleted");
});

Expand All @@ -89,7 +89,7 @@ test.serial("groupResourcesByComponents: theme", async (t) => {
await resourceCollector.visitResource({getPath: () => "/resources/themes/a/.theming", getSize: async () => 13});
t.is(resourceCollector.themePackages.size, 1, "1 theme was added");
await resourceCollector.determineResourceDetails({});
resourceCollector.groupResourcesByComponents({});
resourceCollector.groupResourcesByComponents();
t.is(resourceCollector.themePackages.get("themes/a/").resources.length, 1, "1 theme was grouped");
});

Expand All @@ -111,20 +111,14 @@ test.serial("determineResourceDetails: properties", async (t) => {
getPath: () => "/resources/mylib/i18n/i18n.properties", getSize: async () => 13
});
await resourceCollector.determineResourceDetails({});
resourceCollector.groupResourcesByComponents({});
resourceCollector.groupResourcesByComponents();
const resources = resourceCollector.components.get("mylib/").resources;
t.deepEqual(resources.map((res) => res.i18nName),
[null, "i18n/i18n.properties", "i18n/i18n.properties"], "i18nName was set");
});

test.serial("determineResourceDetails: view.xml", async (t) => {
const resourceCollector = new ResourceCollector({
getModuleInfo: async (moduleInfo) => {
return {
name: "myName"
};
}
});
const resourceCollector = new ResourceCollector();
const enrichWithDependencyInfoStub = sinon.stub(resourceCollector, "enrichWithDependencyInfo")
.returns(Promise.resolve());
await resourceCollector.visitResource({getPath: () => "/resources/mylib/my.view.xml", getSize: async () => 13});
Expand All @@ -133,6 +127,72 @@ test.serial("determineResourceDetails: view.xml", async (t) => {
t.is(enrichWithDependencyInfoStub.getCall(0).args[0].name, "mylib/my.view.xml", "is called with view");
});

test.serial("determineResourceDetails: Debug bundle", async (t) => {
const resourceCollector = new ResourceCollector();

const enrichWithDependencyInfoStub = sinon.stub(resourceCollector, "enrichWithDependencyInfo").resolves();
await resourceCollector.visitResource({getPath: () => "/resources/MyBundle-dbg.js", getSize: async () => 13});

await resourceCollector.determineResourceDetails({
debugBundles: ["MyBundle-dbg.js"]
});
t.is(enrichWithDependencyInfoStub.callCount, 1, "enrichWithDependencyInfo is called once");
t.is(enrichWithDependencyInfoStub.getCall(0).args[0].name, "MyBundle-dbg.js",
"enrichWithDependencyInfo is called with debug bundle");
});

test.serial("determineResourceDetails: Debug files and non-debug files", async (t) => {
const resourceCollector = new ResourceCollector();

const enrichWithDependencyInfoStub = sinon.stub(resourceCollector, "enrichWithDependencyInfo")
.callsFake((resourceInfo) => {
// Simulate enriching resource info with dependency info to test whether it gets shared
// with the dbg resource later on
resourceInfo.dynRequired = true;
});
await Promise.all([
"/resources/MyBundle-dbg.js",
"/resources/mylib/MyControlA-dbg.js",
"/resources/mylib/MyControlA.js",
"/resources/mylib/MyControlB.js",
"/resources/mylib/MyControlB-dbg.js"
].map((resourcePath) => {
return resourceCollector.visitResource({getPath: () => resourcePath, getSize: async () => 13});
}));

await resourceCollector.determineResourceDetails({
debugResources: ["**/*-dbg.js"],
debugBundles: ["MyBundle-dbg.js"]
});
t.is(enrichWithDependencyInfoStub.callCount, 3, "enrichWithDependencyInfo is called three times");
t.is(enrichWithDependencyInfoStub.getCall(0).args[0].name, "MyBundle-dbg.js",
"enrichWithDependencyInfo called with debug bundle");
t.is(enrichWithDependencyInfoStub.getCall(1).args[0].name, "mylib/MyControlA.js",
"enrichWithDependencyInfo called with non-debug control A");
t.is(enrichWithDependencyInfoStub.getCall(2).args[0].name, "mylib/MyControlB.js",
"enrichWithDependencyInfo called with non-debug control B");

t.is(resourceCollector._resources.get("MyBundle-dbg.js").isDebug, true, "MyBundle-dbg is a debug file");
t.is(resourceCollector._resources.get("MyBundle-dbg.js").dynRequired, true,
"MyBundle-dbg is flagged as dynRequired");

t.is(resourceCollector._resources.get("mylib/MyControlA.js").isDebug, false, "MyControlA is no debug file");
t.is(resourceCollector._resources.get("mylib/MyControlA.js").dynRequired, true,
"MyControlA is flagged as dynRequired");

t.is(resourceCollector._resources.get("mylib/MyControlA-dbg.js").isDebug, true, "MyControlA-dbg is a debug file");
t.is(resourceCollector._resources.get("mylib/MyControlA-dbg.js").dynRequired, true,
"MyControlA-dbg is flagged as dynRequired");

t.is(resourceCollector._resources.get("mylib/MyControlB.js").isDebug, false, "MyControlB is no debug file");
t.is(resourceCollector._resources.get("mylib/MyControlB.js").dynRequired, true,
"MyControlB is flagged as dynRequired");

t.is(resourceCollector._resources.get("mylib/MyControlB-dbg.js").isDebug, true, "MyControlB-dbg is a debug file");
t.is(resourceCollector._resources.get("mylib/MyControlB-dbg.js").dynRequired, true,
"MyControlB-dbg is flagged as dynRequired");
});

test.serial("enrichWithDependencyInfo: add infos to resourceinfo", async (t) => {
const resourceCollector = new ResourceCollector({
getModuleInfo: async () => {
Expand Down
26 changes: 8 additions & 18 deletions test/lib/lbt/resources/ResourceInfoList.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,38 +16,28 @@ test("add: add new resources", (t) => {
t.falsy(result.module, "module is not set");
});

test("add: add source then debug resources", (t) => {
test("add: add non-debug resource", (t) => {
const resourceInfoList = new ResourceInfoList("prefix");

resourceInfoList.add({name: "myfile.js", module: "myfile.js", size: 13});
resourceInfoList.add({name: "myfile.js", module: "myfile.js", size: 13, required: new Set(["some-dep.js"])});

const myInfo = {name: "myfile-dbg.js", size: 13};
resourceInfoList.add(myInfo);

t.is(resourceInfoList.resources.length, 2, "two entries");
t.is(resourceInfoList.resources.length, 1, "one resource added");

const result = resourceInfoList.resourcesByName.get("../myfile.js");
t.is(result.module, "myfile.js", "module is set");

const resultDbg = resourceInfoList.resourcesByName.get("../myfile-dbg.js");
t.is(resultDbg.module, "myfile.js", "module is set");
t.deepEqual(result.required, new Set(["some-dep.js"]), "module is set");
});

test("add: add debug then source resources", (t) => {
test("add: add debug resources", (t) => {
const resourceInfoList = new ResourceInfoList("prefix");

resourceInfoList.add({name: "myfile-dbg.js", size: 13});
resourceInfoList.add({name: "myfile-dbg.js", size: 13, required: new Set(["some-dep.js"])});

const myInfo = {name: "myfile.js", module: "myfile.js", size: 13};
resourceInfoList.add(myInfo);

t.is(resourceInfoList.resources.length, 2, "two entries");

const result = resourceInfoList.resourcesByName.get("../myfile.js");
t.is(result.module, "myfile.js", "module is set");
t.is(resourceInfoList.resources.length, 1, "one resource added");

const resultDbg = resourceInfoList.resourcesByName.get("../myfile-dbg.js");
t.is(resultDbg.module, "myfile.js", "module is set");
t.deepEqual(resultDbg.required, new Set(["some-dep.js"]), "module is set");
});

test("add: add i18n resource", (t) => {
Expand Down