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

RandomByte · 2020-11-24T18:57:42Z

Stop analyzing debug-resources if they correspond to a non-debug resource.

Only analyze the non-debug resource and copy any dependency information to the corresponding debug-resource.

While this might not be the best approach given that analyzing the source ("debug") version of a resource might result in finding more dependencies (i.e. those mangled during minification), this change ensures that we generate the same resources.json, independently from the order of debug and non-debug resources supplied to the resourceListCreator processor.

Resolves SAP/ui5-tooling#274

coveralls · 2020-11-24T19:05:28Z

Coverage increased (+0.005%) to 92.96% when pulling 44af975 on resources-json-generation-consistency into e9458e7 on master.

RandomByte · 2020-11-24T19:07:01Z

Obviously still missing dedicated tests for the added code and parameters.

We could not measure any immediate performance improvement.

We also discussed alternative implementations. Including one even more similar to how Maven does it, where we generate the resources.json information during the bundling (where no debug files exist yet) and enrich it with the debug file information at a later point. Either while they are being created or in a separate task. But this would be a bigger effort than what this PR does.

codeworrior · 2020-11-25T07:46:12Z

lib/lbt/resources/ResourcePool.js

 		let info = this._dependencyInfos.get(name);
 		if ( info == null ) {
 			// console.log("analyzing ", name);
+			let dbgResource;
+			if (options && options.preferDebugResources) {


This feels somehow wrong. Complicating the ResourcePool for a late task by adding difficult to understand logic that's not needed in earlier tasks.

Maybe thinking of tasks in a more holistic way would be better. Currently, we see tasks as modifying or creating files, but maybe they rather should be transformations of the project (or the projects build results) into another, consistent build result.

With that interpretation, an early task would determine dependencies on the original sources and store them in an early version of the resources.json.

createDebugFiles, when executed, would have to update that file. The logic which files will be copied and which not, would then be isolated in the createDebugFiles task (or processor, I don't know) and not being spread across tasks.

All bundling task obviously also would have to update any existing resources.json.

Well, but even with that approach, encapsulation would not be a no-brainer. All the mentioned tasks would have to know where a resources.json files can exist (library, component, theme).

codeworrior · 2020-11-25T07:50:18Z

lib/lbt/resources/ResourcePool.js

+				// not-minified content of a resource.
+				const dbgName = ResourceInfoList.getDebugName(name);
+				if (dbgName && (!options.debugBundleFilter || !options.debugBundleFilter.matches(dbgName))) {
+					dbgResource = this._resourcesByName.get(dbgName);


Making findResource async was meant as a preparation to later switch from a prepare and wrap approach to a direct access via the fs adapters. But I guess you wanted to avoid the (ugly) try catch.

Maybe that's the best approach then? Rewriting the ResourcePool to tightly integrate with ui5-fs. Also, adding a new Class in ui5-fs to store metadata like the AST (or plain dependency information?) per-resource.

Every task modifying a resource should update this metadata or at least invalidate it in some way.

Then there would still be a final resource.json-generation task, but instead of analyzing resources it would use the metadata already available for most of them.

codeworrior · 2020-11-25T07:58:49Z

lib/lbt/resources/ResourceCollector.js

-				promises.push(
-					this.enrichWithDependencyInfo(info)
-				);
+				if ( (!info.isDebug || debugBundleFilter.matches(name)) ) {


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?

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

…stic Stop analyzing debug-resources if they correspond to a non-debug resource. Only analyze the non-debug resource and copy any dependency information to the corresponding debug-resource. While this might not be the best approach given that analyzing the source ("debug") version of a resource might result in finding more dependencies (i.e. those mangled during minification), this change ensures that we generate the same resources.json, independently from the order of debug and non-debug resources supplied to the resourceListCreator processor. Resolves SAP/ui5-tooling#274

… -bundles

…e content Possible follow up of #551 to improve dependency analysis. JSModuleAnalyzer has trouble detecting some dependencies in minified code if they have been "mangled". E.g. in case jQuery is not named jQuery anymore. This change solves that by trying to retrieve the debug ("source") content of a resource to analyze that instead of the minified version. However, integrating this on the ResourcePool level is not ideal and a more general solution to this problem should be preferred. Therefore this PR should only act as an illustration of the discussions and thoughts. Currently it should not be merged.

tobiasso85

LGTM

matz3 · 2020-11-30T10:11:54Z

lib/lbt/resources/ResourceCollector.js

-					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)


Suggested change

// Only analyze non-debug files which are not special debug bundles (like sap-ui-core-dbg.js)

// Only analyze non-debug files and special debug bundles (like sap-ui-core-dbg.js)

or am I wrong?

Absolutely right 👍

matz3 · 2020-11-30T10:14:24Z

lib/lbt/resources/ResourceCollector.js

@@ -253,19 +272,17 @@ class ResourceCollector {

 	groupResourcesByComponents(options) {


options could be removed completely, right?

matz3 · 2020-11-30T10:16:03Z

test/lib/lbt/resources/ResourceCollector.js

+	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


Suggested change

// with the dbg resource alter on

// with the dbg resource later on

matz3 · 2020-11-30T10:19:54Z

lib/lbt/resources/ResourceCollector.js

+			newDbgInfo.copyFrom(null, nonDbgInfo);
+			newDbgInfo.copyFrom(null, dbgInfo);


Suggested change

newDbgInfo.copyFrom(null, nonDbgInfo);

newDbgInfo.copyFrom(null, dbgInfo);

newDbgInfo.copyFrom(null, nonDbgInfo); // First copy info of analysis from non-dbg file (included, required, condRequired, ...)

newDbgInfo.copyFrom(null, dbgInfo); // Then copy over info from dbg file to properly set name, isDebug, ...

codeworrior

LGTM

RandomByte requested review from codeworrior, matz3 and tobiasso85 November 24, 2020 18:57

RandomByte force-pushed the resources-json-generation-consistency branch from 9093861 to 245b592 Compare November 24, 2020 19:03

codeworrior reviewed Nov 25, 2020

View reviewed changes

RandomByte force-pushed the resources-json-generation-consistency branch from 245b592 to 374e0c0 Compare November 26, 2020 14:40

RandomByte added 2 commits November 26, 2020 15:42

[INTERNAL] ResourceCollector: Fix existing tests

7ec38a4

RandomByte force-pushed the resources-json-generation-consistency branch 2 times, most recently from a279872 to 7ec38a4 Compare November 26, 2020 14:45

[INTERNAL] ResourceCollector: Add tests for handling of dbg files and…

40a9032

… -bundles

RandomByte marked this pull request as ready for review November 26, 2020 16:06

RandomByte mentioned this pull request Nov 26, 2020

[PoC] ResourcePool: Add option to prefer analyzing a resource's source content #553

Draft

[INTERNAL] ResourceInfoList: Remove obsolete function getDebugName

6bbc8e9

tobiasso85 previously approved these changes Nov 30, 2020

View reviewed changes

matz3 requested changes Nov 30, 2020

View reviewed changes

[INTERNAL] Apply suggestions from code review

44af975

RandomByte dismissed tobiasso85’s stale review via 44af975 November 30, 2020 12:45

RandomByte requested review from matz3, codeworrior and tobiasso85 November 30, 2020 12:46

matz3 approved these changes Nov 30, 2020

View reviewed changes

codeworrior approved these changes Nov 30, 2020

View reviewed changes

RandomByte merged commit 26eb89a into master Nov 30, 2020

RandomByte deleted the resources-json-generation-consistency branch November 30, 2020 14:23

	// Only analyze non-debug files which are not special debug bundles (like sap-ui-core-dbg.js)
	// Only analyze non-debug files and special debug bundles (like sap-ui-core-dbg.js)

		@@ -253,19 +272,17 @@ class ResourceCollector {

		groupResourcesByComponents(options) {

	// with the dbg resource alter on
	// with the dbg resource later on

		newDbgInfo.copyFrom(null, nonDbgInfo);
		newDbgInfo.copyFrom(null, dbgInfo);

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

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

Uh oh!

Conversation

RandomByte commented Nov 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RandomByte commented Nov 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasso85 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codeworrior left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RandomByte commented Nov 24, 2020 •

edited

Loading

coveralls commented Nov 24, 2020 •

edited

Loading