-
Notifications
You must be signed in to change notification settings - Fork 24
[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
Changes from 4 commits
b9d397e
7ec38a4
40a9032
6bbc8e9
44af975
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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) ) { | ||||||||||
|
|
@@ -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)) ) { | ||||||||||
| // Only analyze non-debug files which are not special debug bundles (like sap-ui-core-dbg.js) | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
or am I wrong?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Absolutely right 👍 |
||||||||||
| promises.push( | ||||||||||
| this.enrichWithDependencyInfo(info) | ||||||||||
| ); | ||||||||||
| } else { | ||||||||||
| nonBundledDebugResources.push(info); | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // set the module name for .properties and .json | ||||||||||
|
|
@@ -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}'`); | ||||||||||
|
|
@@ -226,7 +235,17 @@ 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); | ||||||||||
| newDbgInfo.copyFrom(null, nonDbgInfo); | ||||||||||
| newDbgInfo.copyFrom(null, dbgInfo); | ||||||||||
|
Comment on lines
+245
to
+249
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||||||||||
| this._resources.set(dbgInfo.name, newDbgInfo); | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| createOrphanFilters() { | ||||||||||
|
|
@@ -253,19 +272,17 @@ class ResourceCollector { | |||||||||
|
|
||||||||||
| groupResourcesByComponents(options) { | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes 🙌 |
||||||||||
| 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; | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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"); | ||||||
| }); | ||||||
|
|
||||||
|
|
@@ -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"); | ||||||
| }); | ||||||
|
|
||||||
|
|
@@ -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}); | ||||||
|
|
@@ -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 alter on | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||||||
| 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 () => { | ||||||
|
|
||||||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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