[FEATURE] Add generateResourcesJson task #390

codeworrior · 2019-12-29T11:42:18Z

Introduces new task "generateResourcesJson" which creates the file resources.json for libraries, applications and themes.
The task analyses the resources and collects meta information and stores them in a file called resources.json.
This meta information includes module name, size, dependencies, includes, ...

coveralls · 2019-12-29T11:44:04Z

Coverage increased (+0.7%) to 92.279% when pulling a58c11e on create-resources-json into c47c8c6 on master.

codeworrior · 2019-12-29T21:50:53Z

Note: failing test requires #391.

codeworrior · 2019-12-29T21:53:52Z

lib/processors/resourceListCreator.js

+
+	getProject() {
+		return this.resource._project;
+	}


This method has been added in the moduleBundler, but I couldn't find a local usage within ui5-builder. However, code-coverage says, it is used. Do you know where?

it is not called! Will remove it.

tasks › bundlers › generateStandaloneAppBundle.integration › integration: build application.b standalone Rejected promise returned by test. Reason: TypeError { message: 'resource.getProject is not a function', } › module.exports (lib/lbt/utils/escapePropertiesFile.js:14:27) › BundleBuilder.writePreloadModule (lib/lbt/bundle/Builder.js:432:30) › BundleBuilder.writePreloadFunction (lib/lbt/bundle/Builder.js:288:17) › BundleBuilder._createBundle (lib/lbt/bundle/Builder.js:151:4) › Object.module.exports [as task] (lib/tasks/bundlers/generateStandaloneAppBundle.js:84:2) › Object.build (lib/builder/builder.js:368:4)

the original one gets called, but the copied one didn't

codeworrior · 2019-12-29T21:55:51Z

lib/processors/resourceListCreator.js

+	return path.slice( "/resources/".length);
+}
+
+class LocatorResource extends Resource {


do we have best practices how to share code between processors? They so far seem to be self-contained? But I need the LBT pool now in the resourceListCreator, too. I copied the LocatorResource and LocatorResourcePool for now, but that's not really nice.

resourceListCreator and moduleBundler use it.

How about a subfolder in processors -> processors/lib for helpers and shared code between processors?
Note: This was done the same way for jsdoc -> processors/jsdoc/lib

codeworrior · 2019-12-29T21:58:12Z

lib/tasks/generateResourcesJson.js

+				resources.push(res);
+			}
+		});
+	} else {


This felt strange to me. Is there no easier (and official) way to get a list of all resources of the current project? workspace.byGlobSource only returns source files, no generated files, a search with the namespace pattern doesn't work for sap.ui.core or for apps without a namespace.

workspace.byGlob also only returns resources of the current project. Searching with the namespace should be possible by now (I know your comment is from last year), since with v2.0 all projects are required to have a namespace.

And what about sap.ui.core? Yet another, hard-coded exception?

Yes, now I get it. Indeed for sap.ui.core the namespace sap/ui/core is insufficient. A glob pattern like to current one should be fine though.

Still, why do we use byGlobSource instead of byGlob? I thought we are also interested in generated files.

byGlobSource retrieves the debug files as well.

byGlobSource retrieves the debug files as well.

Are you sure? I think that should not be the case unless they already exist in the source directory.

you're right, byGlob retrieves all the files.

adjusted logic for copy from source such that it does not depend on the combination of byGlobSource and adding missing ones from byGlob on top.
f346f3e

codeworrior · 2019-12-29T22:00:54Z

lib/processors/resourceListCreator.js

+			}
+			// a .theme file within a theme folder indicates a library/theme package
+			// Note: ignores .theme files in library folders
+			if ( name.match("(?:[^/]+/)*themes/[^/]+/(?:\\.theming|library.source.less)") && !this._themePackages.has(prefix) ) {


In the Maven tooling, the library.source.less was not used as a marker, but the ui5-tooling does not create the expected .theming files. @matz3 What is the right approach here? Should we create the .theming files or chose a different approach?

We need to create the files in order to support the Theming Engine. This becomes relevant when our main delivery is built with the UI5 Tooling.

For now the library.source.less as an indicator is IMO fine. We also use it within the theme build as the files are required for the build (which was not the case in the legacy Maven build).

As we generate now .theming files, we could omit the library.source.less, if and only if the task execution order is appropriate.

We only do so in the internal theme build (which produces the current delivery for SAPUI5 / OpenUI5 (except the npm packages).
So I think we need to stay with library.source.less for the time being.

codeworrior · 2019-12-29T22:02:32Z

lib/processors/resourceListCreator.js

+		if ( options.failOnOrphans ) {
+			throw new Error("not all resources could be assigned to components");
+		}
+	}


Is there a guideline where errors should be thrown, processors or tasks?

I think the lowest level is here perfect because we have all the information there and typically the error should bubble all the way up

That's fine, but the error message should try to explain the context better. The call stack might get lost through catching and wrapping this error while bubbling up.

Some good examples:

https://github.com/SAP/ui5-builder/blob/master/lib/processors/uglifier.js#L27

https://github.com/SAP/ui5-builder/blob/master/lib/processors/bundlers/flexChangesBundler.js#L66

codeworrior · 2019-12-29T22:03:28Z

lib/tasks/generateResourcesJson.js

+	"!**/.eslintrc",
+	"!**/.eslintignore",
+	"!**/.gitignore"
+];


Do we have a common place for such excludes?

Not yet. Although I would not expect the last three in the webapp or src directories. Do we know of such cases?

Would it be possible to rather have an include on the expected file extensions?

the file extensions can be anything required by the application, from image to web resource, therefore it is hard to define a whitelist.

codeworrior · 2020-06-25T04:29:14Z

lib/processors/resourceListCreator.js

@@ -113,6 +124,21 @@ class ResourceInfo {
 		if ( this.included != null && this.included.size > 0 ) {
 			this.merged = true;
 		}
+		if (orig.size > 0) {


as the default now is -1, shouldn't the check be for orig.size >= 0 ?

good point :) just found it in the original sources.

yes, there it's orig.size >= 0, too.

codeworrior · 2020-06-25T04:49:18Z

lib/lbt/LocatorResourcePool.js

+		return Promise.all(
+			resources.map(
+				(resource) => this.addResource( new LocatorResource(this, resource) )
+			).filter( (followUp) => followUp )


just Boolean

codeworrior · 2020-06-25T04:50:11Z

lib/lbt/LocatorResourcePool.js

+				(resource) => this.addResource( new LocatorResource(this, resource) )
+			).filter( (followUp) => followUp )
+		);
+		// .then( () => {


Commented code can be deleted...

codeworrior · 2020-06-25T04:59:16Z

lib/processors/resourceListCreator.js

+			}
+			// a .theme file within a theme folder indicates a library/theme package
+			// Note: ignores .theme files in library folders
+			if ( name.match("(?:[^/]+/)*themes/[^/]+/(?:\\.theming|library.source.less)") && !this._themePackages.has(prefix) ) {


As we generate now .theming files, we could omit the library.source.less, if and only if the task execution order is appropriate.

codeworrior · 2020-06-25T05:33:36Z

lib/processors/resourceListCreator.js

+	} catch (e) {
+		// ignore error
+	}
+


Isn't this the most expensive way to determine the size of the resource?

getString() has to read the resource, convert the UTF-8 byte stream to a Unicode16 char sequence; Buffer.byteLength then has to do the opposite operation just to determine the original UTF-8 byte length.

Overall performance when building sap.ui.core also looks very bad.

hm already counted the buffer length but that's not correct, have to find a better way.
The statInfo would be the best, but this needs to be updated when the sources are compressed

why / how was the buffer length not correct?

Ok, Buffer.prototype.length is the allocation size, not the content length. Too bad. To me that means, @ui5/fs.Resource needs to know the size, if neither statInfo nor Buffer are suitable to determine it.

Maybe for
@ui5/fs.Resource
we can use the stats.size property for size
but we must ensure that each content modification will trigger a size update

Exactly, maintaining the (byte) size IMO should become a responsibility of the Resource, to get a proper abstraction.

Downside: when new content is provided as a string, this unfortunately requires either encoding the string as UTF-8 or using Buffer.byteLength. Maybe that update of the size then should be done lazily.

I agree the Resource should be responsible.
If a string is provided it is anyways converted to a buffer and the buffer.byteLength is just a property so there should be no performance impact.

can use statinfo size with
SAP/ui5-fs#253

codeworrior · 2020-06-25T05:37:03Z

lib/tasks/generateResourcesJson.js

+		resources = await workspace.byGlob(["/resources/**/*.*", ...DEFAULT_EXCLUDES]);
+	}
+	const dependencyResources = await dependencies.byGlob("/resources/**/*.{js,json,xml,html,properties,library}");
+


I have no idea, why I added the resources from dependencies in my initial commit.

Thinking about it now, the resources.json is a library-local thing (or component-local thing) and should not include the dependencies. They have their own resources.json.

Wanted to verify the impact by building sap.ui.layout with dependencies. But building it failed in sap.ui.core, because sap.ui.core has modules outside its namespace -> orphans.

done with d97bbe8

Update: Analysis of XMLViews and fragments requires the resources from dependencies. That was one reason t odd them.

RandomByte · 2020-08-03T20:05:02Z

lib/tasks/generateResourcesJson.js

+				resources.push(res);
+			}
+		});
+	} else {


Yes, now I get it. Indeed for sap.ui.core the namespace sap/ui/core is insufficient. A glob pattern like to current one should be fine though.

Still, why do we use byGlobSource instead of byGlob? I thought we are also interested in generated files.

RandomByte · 2020-08-03T20:08:03Z

lib/tasks/generateResourcesJson.js

+	"!**/.eslintrc",
+	"!**/.eslintignore",
+	"!**/.gitignore"
+];


Not yet. Although I would not expect the last three in the webapp or src directories. Do we know of such cases?

Would it be possible to rather have an include on the expected file extensions?

RandomByte · 2020-08-03T20:09:39Z

package.json

@@ -21,7 +21,7 @@
 		"test": "npm run lint && npm run jsdoc-generate && npm run coverage",
 		"test-azure": "npm run lint && npm run jsdoc-generate && npm run coverage-xunit",
 		"lint": "eslint ./",
-		"unit": "rimraf test/tmp && ava",
+		"unit": "rimraf test/tmp && ava -v",


That's what the unit-verbose npm script one line below is supposed for. Or was this changed for some temporary test?

was just temporary, this change somehow slided in, will revert this line

RandomByte · 2020-08-03T20:14:53Z

lib/processors/resourceListCreator.js

+		if ( options.failOnOrphans ) {
+			throw new Error("not all resources could be assigned to components");
+		}
+	}


That's fine, but the error message should try to explain the context better. The call stack might get lost through catching and wrapping this error while bubbling up.

Some good examples:

https://github.com/SAP/ui5-builder/blob/master/lib/processors/uglifier.js#L27

https://github.com/SAP/ui5-builder/blob/master/lib/processors/bundlers/flexChangesBundler.js#L66

RandomByte · 2020-08-07T17:01:10Z

I think I won't be able to review the functional correctness of this PR in coming week. I simply don't know enough about what this task does and should do.

I'm surprised by the amount of new test fixtures. But for the same reason as above I can't tell whether they are needed or not.

But I tried something that might be interesting: I removed the new task execution from ApplicationBuilder.js, LibraryBuilder.js and ThemeLibraryBuilder.js. With that change, the code coverage for ResourceCollector.js, ResourceInfo.js and resourceListCreator.js went from almost 100% to below 70% each. This gives me the impression that some of the new functions are only tested through integration tests and do not have dedicated unit tests:

For new modules we typically try to have at least one unit test per function. First, to efficiently test a variety of inputs. And secondly, to have failing tests which already contain the name of faulty function instead of a more generic integration test which first needs to be debugged on its own to identify the issue.

RandomByte · 2020-08-08T12:16:51Z

Building the openui5-sample-app with this change currently fails with an error:

⚠️  Process Failed With Error

Error Message:
resources.json generation failed with error: There are 1 resources which could not be assigned to components.

Stack Trace:
Error: resources.json generation failed with error: There are 1 resources which could not be assigned to components.
    at module.exports (/ui5-builder/lib/processors/resourceListCreator.js:183:9)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at async Object.build (/ui5-builder/lib/builder/builder.js:370:4)
    at async Object.handleBuild [as handler] (/ui5-cli/lib/cli/commands/build.js:97:2)

The errors logged during the build are:

ERR! lbt:resources:ResourcePool TypeError: Cannot read property 'type' of undefined
ERR! lbt:resources:ResourcePool     at new SapUiDefineCall (/ui5-builder/lib/lbt/calls/SapUiDefine.js:34:16)
ERR! lbt:resources:ResourcePool     at XMLCompositeAnalyzer.analyze (/ui5-builder/lib/lbt/analyzer/XMLCompositeAnalyzer.js:19:23)
ERR! lbt:resources:ResourcePool     at determineDependencyInfo (/ui5-builder/lib/lbt/resources/ResourcePool.js:71:35)
ERR! lbt:resources:ResourcePool     at processTicksAndRejections (internal/process/task_queues.js:93:5)
ERR! lbt:resources:ResourcePool     at async LocatorResourcePool.getModuleInfo (/ui5-builder/lib/lbt/resources/ResourcePool.js:175:11)
ERR! lbt:resources:ResourcePool     at async Promise.all (index 9)
ERR! lbt:resources:ResourcePool     at async module.exports (/ui5-builder/lib/processors/resourceListCreator.js:117:2)
ERR! lbt:resources:ResourcePool     at async Object.build (/ui5-builder/lib/builder/builder.js:370:4)
ERR! lbt:resources:ResourcePool     at async Object.handleBuild [as handler] (/ui5-cli/lib/cli/commands/build.js:97:2)
ERR! lbt:resources:ResourcePool  failed to parse or analyze %s: sap/ui/demo/todo/test/unit/AllTests.js TypeError: Cannot read property 'type' of undefined
ERR! lbt:resources:ResourcePool     at new SapUiDefineCall (/ui5-builder/lib/lbt/calls/SapUiDefine.js:34:16)
ERR! lbt:resources:ResourcePool     at XMLCompositeAnalyzer.analyze (/ui5-builder/lib/lbt/analyzer/XMLCompositeAnalyzer.js:19:23)
ERR! lbt:resources:ResourcePool     at determineDependencyInfo (/ui5-builder/lib/lbt/resources/ResourcePool.js:71:35)
ERR! lbt:resources:ResourcePool     at processTicksAndRejections (internal/process/task_queues.js:93:5)
ERR! lbt:resources:ResourcePool     at async LocatorResourcePool.getModuleInfo (/ui5-builder/lib/lbt/resources/ResourcePool.js:175:11)
ERR! lbt:resources:ResourcePool     at async Promise.all (index 9)
ERR! lbt:resources:ResourcePool     at async module.exports (/ui5-builder/lib/processors/resourceListCreator.js:117:2)
ERR! lbt:resources:ResourcePool     at async Object.build (/ui5-builder/lib/builder/builder.js:370:4)
ERR! lbt:resources:ResourcePool     at async Object.handleBuild [as handler] (/ui5-cli/lib/cli/commands/build.js:97:2)
ERR! lbt:resources:ResourcePool TypeError: Cannot read property 'type' of undefined
ERR! lbt:resources:ResourcePool     at new SapUiDefineCall (/ui5-builder/lib/lbt/calls/SapUiDefine.js:34:16)
ERR! lbt:resources:ResourcePool     at XMLCompositeAnalyzer.analyze (/ui5-builder/lib/lbt/analyzer/XMLCompositeAnalyzer.js:19:23)
ERR! lbt:resources:ResourcePool     at determineDependencyInfo (/ui5-builder/lib/lbt/resources/ResourcePool.js:71:35)
ERR! lbt:resources:ResourcePool     at processTicksAndRejections (internal/process/task_queues.js:93:5)
ERR! lbt:resources:ResourcePool     at async LocatorResourcePool.getModuleInfo (/ui5-builder/lib/lbt/resources/ResourcePool.js:175:11)
ERR! lbt:resources:ResourcePool     at async Promise.all (index 23)
ERR! lbt:resources:ResourcePool     at async module.exports (/ui5-builder/lib/processors/resourceListCreator.js:117:2)
ERR! lbt:resources:ResourcePool     at async Object.build (/ui5-builder/lib/builder/builder.js:370:4)
ERR! lbt:resources:ResourcePool     at async Object.handleBuild [as handler] (/ui5-cli/lib/cli/commands/build.js:97:2)

RandomByte · 2020-08-08T13:14:23Z

I also did some benchmarking using:

OpenUI5 sap.m library

ui5-builder ref	Command	Mean [s]	Min [s]	Max [s]
master (`e7d4431`)	`ui5 build`	24.354 ± 2.094	21.244	26.661
create-resources-json (`18fb726`)	`ui5 build`	27.820 ± 1.174	26.073	30.163

OpenUI5 Sample App (+ 6 dependencies)

(skipping the generateResourcesJson task for applications due to the error above)

ui5-builder ref	Command	Mean [s]	Min [s]	Max [s]
master (`e7d4431`)	`ui5 build --all`	65.268 ± 2.128	60.574	68.159
create-resources-json (`18fb726`)	`ui5 build --all`	74.221 ± 1.947	70.642	77.307

Benchmark tool: hyperfine with one warmup
Hardware: MacBook Pro 2017 - i7 2,9GHz/16GB

codeworrior · 2020-08-08T20:41:53Z

The error seen during the build of the openui5-sample-app should be fixed with #491.

RandomByte · 2020-08-10T09:04:22Z

@codeworrior and I discussed the benchmark results:

The resources.json is not consumed at runtime. Only some known tools make use of the file.
- This means many UI5 Tooling consumer do not need it
Both measurements show an increase in runtime of about 14%.
This task might fail for some projects that are currently working fine
- For example a library with multiple namespaces would need a way to configure those (currently not possible). Otherwise the build will likely fail

Both, @codeworrior and I therefore think it would be better to skip this task by default, similar to the generateManifestBundle task. For this it needs to be added to the list here: https://github.com/SAP/ui5-builder/blob/master/lib/builder/builder.js#L54

Btw. should we document the structure of the resources.json somewhere? Do we expect other tools to make use of it? I'm not sure.

RandomByte · 2020-08-10T11:47:31Z

test/lib/builder/builder.js

@@ -277,6 +277,9 @@ test("Build application.g", (t) => {
 	const expectedPath = path.join("test", "expected", "build", "application.g", "dest");

 	return builder.build({
+		includedTasks: [


Is there any integration test left that does not execute the generateResourceJson task now?

distributed it more

RandomByte · 2020-08-10T13:06:42Z

test/lib/builder/builder.js

@@ -86,14 +94,14 @@ test("Build application.a", (t) => {
 	return builder.build({
 		tree: applicationATree,
 		destPath,
-		excludedTasks: ["generateComponentPreload", "generateStandaloneAppBundle", "generateVersionInfo"]
+		excludedTasks: ["generateComponentPreload", "generateStandaloneAppBundle", "generateVersionInfo", "generateResourcesJson"]


"generateResourcesJson" is not default anymore and therefore does not need to be excluded here and in following tests

RandomByte · 2020-08-10T13:09:26Z

test/lib/builder/builder.js

@@ -496,6 +554,9 @@ test("Build theme.j even without an library", (t) => {
 	const destPath = "./test/tmp/build/theme.j/dest";
 	const expectedPath = "./test/expected/build/theme.j/dest";
 	return builder.build({
+		includedTasks: [


Please keep at least one default theme build

codeworrior · 2020-08-11T08:03:38Z

@RandomByte Reg. your comment:

Btw. should we document the structure of the resources.json somewhere? Do we expect other tools to make use of it? I'm not sure.

To be honest, I didn't remember this, but a search in our Wiki revealed an existing documentation in the SDK.

The documented entries had been the "public" ones at the time of writing. But I guess we could enhance the documentation with a few more properties. A JSON schema also could make sense, but low priority as no one would use it currently (and the file is not intended to be maintained manually).

RandomByte · 2020-08-11T15:49:38Z

lib/tasks/generateResourcesJson.js

+}
+
+/**
+ * Task for creating a resources.json file, describing all productive build resources.


Should we add a link to the relevant documentation: https://openui5.hana.ondemand.com/#topic/adcbcf8b50924556ab3f321fcd9353ea ?

RandomByte · 2020-08-11T15:57:08Z

test/fixtures/library.n/src/library/n/Button.js

+				sap.ui.requireSync("./changeHandler/SplitButton");
+				oRm.openStart("button", oButton);
+				oRm.class("libNBtnBase");
+				oRm.openEnd();


Why is there so much code in some of these fixtures? How is this relevant to the bundling/resources.json generation?

It's seemingly copied from somewhere. But I couldn't find it in the OpenUI5 repository. What's the origin of this code here and in the other new fixtures?

removed unnecessary code lines, I couldn't find the origin either.
I think the main purpose was to have dynamic dependencies nested in the ast

RandomByte · 2020-08-11T16:11:59Z

I cherry-picked the commits from #491 onto this branch and tried building the openui5-sample-app again using ui5 build --include-task=generateResourcesJson --verbose.

It fails with a different error now:

[...]
verb lbt:analyzer:ComponentAnalyzer adding library dependency  sap/m/library.js false
verb lbt:analyzer:ComponentAnalyzer adding library dependency  sap/f/library.js false
verb lbt:analyzer:ComponentAnalyzer adding library dependency  sap/ui/unified/library.js false
verb lbt:analyzer:ComponentAnalyzer derived model implementation dependency  sap/ui/model/resource/ResourceModel.js
verb lbt:analyzer:ComponentAnalyzer derived model implementation dependency  sap/ui/model/json/JSONModel.js
verb lbt:resources:ResourceCollector   configured external resources filters (resources outside the namespace): (none)
verb lbt:resources:ResourceFilterList filetypes: undefined
verb lbt:resources:ResourceFilterList   sap-ui-core-dbg.js --> include: "sap-ui-core-dbg.js"
verb builder:processors:resourceListCreator     writing 'sap/ui/demo/todo/resources.json'
ERR! builder:processors:resourceListCreator resources.json generation failed because of unassigned resources: sap-ui-version.json
ERR! builder:builder Build failed in 523 ms
info builder:builder Executing cleanup tasks...

⚠️  Process Failed With Error

Error Message:
resources.json generation failed with error: There are 1 resources which could not be assigned to components.

Stack Trace:
Error: resources.json generation failed with error: There are 1 resources which could not be assigned to components.
    at module.exports (/ui5-builder/lib/processors/resourceListCreator.js:198:9)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at async Object.module.exports [as task] (/ui5-builder/lib/tasks/generateResourcesJson.js:88:24)
    at async Object.build (/ui5-builder/lib/builder/builder.js:382:4)
    at async Object.handleBuild [as handler] (/ui5-cli/lib/cli/commands/build.js:97:2)

This seems to be related to sap-ui-version.json.

I also don't really like that the ultimate error message is the same as before, even though it's seemingly caused by something else(?)
This will be the error message people will open issues with. Currently it doesn't seem to be very helpful to us.

Edit: I just found that the error log resources.json generation failed because of unassigned resources: sap-ui-version.json was also present in the last case. So maybe #491 actually fixed a different issue? Possibly an issue which actually would not have failed the build?

RandomByte · 2020-08-11T16:34:41Z

test/lib/builder/builder.js

+		],
+		tree: applicationJTree,
+		destPath,
+		excludedTasks: ["createDebugFiles", "generateStandaloneAppBundle", "generateVersionInfo"]


If we would not exclude generateVersionInfo here, this test would reproduce the issue I reported for the openui5-sample-app: #390 (comment)

this seems related to the resource path:
"other resources": /resources/application/j/manifest.json
"sap-ui-version.json": /resources/sap-ui-version.json
--> the prefix /resources/application/j/ is expected by the grouping logic

sap-ui-version.json was excluded with edbf669
It is not relevant for the resources.json and should not be considered.

Will other files with a path like /resources/myfile.xyz still break the build?

yes this currently breaks it.
But this is typically the result of a custom task, "normal" resources should reside in /resources/application/j folder

codeworrior

Stupid Github: as I'm an author, I'm not allowed to "Request changes".

codeworrior · 2020-08-11T16:33:27Z

lib/lbt/analyzer/JSModuleAnalyzer.js

 			const args = node.arguments;
 			const nArgs = args.length;
 			const i = 0;

 			if ( i < nArgs ) {
 				if ( isString(args[i]) ) {
-					const moduleName = ModuleName.fromRequireJSName( args[i].value );
+					// resolve dependencies absolute: e.g. "library/n/changeHandler/SplitButton.js"


Where does this change come from? sap.ui.requireSync does not support relative dependencies? Seems wrong to me.

my bad, didn't know that

sap.ui.requireSync("./Control") ui5loader-dbg.js:468 Uncaught Error: relative name not supported ('./Control.js'

codeworrior · 2020-08-11T16:34:08Z

lib/lbt/analyzer/JSModuleAnalyzer.js

@@ -404,17 +442,24 @@ class JSModuleAnalyzer {
 				} else if ( isMethodCall(node, CALL_REQUIRE_SYNC) || isMethodCall(node, CALL_SAP_UI_REQUIRE_SYNC) ) {
 					// recognizes a call to sap.ui.requireSync
 					info.setFormat(ModuleFormat.UI5_DEFINE);
-					onSapUiRequireSync(node, conditional);
+
+					onSapUiRequireSync(node, conditional, info.name);


Seems wrong to me, see below

done :)
-> above

codeworrior · 2020-08-11T16:46:57Z

lib/lbt/resources/ResourcesList.js

@@ -0,0 +1,153 @@
+const ResourceInfo = require("./ResourceInfo");


This module here should rather be named ResourceInfoList, as it otherwise could be confused with a list of Resource.js objects.

done
(was also surprised by the plural "Resources" + "List")

codeworrior · 2020-08-11T16:50:57Z

lib/lbt/resources/ResourceCollector.js

+ */
+class ResourceCollector {
+	/**
+	 * Collects resources


"Collects a set of ResourceInfo objects and groups them by components, libraries and themes."

codeworrior · 2020-08-11T16:54:52Z

lib/processors/resourceListCreator.js

+
+	const collector = new ResourceCollector(pool);
+	const visitPromises = resources.map((resource) => {
+		return resource.getSize().then((size) => {


now hat the size became a property of the resource, couldn't the getSize() be integrated in the collector?

codeworrior · 2020-08-11T16:57:58Z

lib/processors/resourceListCreator.js

+ * @returns {string} new content with resources.json entry
+ */
+function addResourcesJsonToList(list, prefix) {
+	// having the file size entry part of the file is a bit like the chicken egg scenario


I find the function name quite misleading. The main purpose of the function seems to be that it creates a JSON string for the given ResourceInfoList. The fact that it also adds the resources.json to the list itself, is an (important) detail on top.

Maybe rather name it makeResourcesJSON....

codeworrior · 2020-08-11T17:12:47Z

lib/tasks/generateResourcesJson.js

+				"**/library-all-dbg.js",
+				"**/designtime/library-preload.designtime.js",
+				"**/library-preload.support.js"
+			].join(",")


I thought I had commented on this earlier, but can't find it.

The representation of the filters as comma separated lists is a legacy from the Maven tooling. In a Maven Mojo, those lists have been configured as comma separated lists. But here, in a JavaScript task API, this doesn't make much sense. I would rather switch to pure arrays.

But as long as this is an internal API (as it is currently), it doesn't matter and we could refactor this later.

the rebase -> force push changed the history o0 maybe it was lost/hard to find due to that

Excluded version info files since they are not part of the resources.json

matz3 · 2020-08-20T09:53:45Z

lib/tasks/bundlers/generateLibraryPreload.js

@@ -48,7 +48,7 @@ function getBundleDefinition(namespace) {

 						// include only thirdparty that is very likely to be used
 						"sap/ui/thirdparty/crossroads.js",
-						"sap/ui/thirdparty/caja-htmlsanitizer.js",
+						"sap/ui/thirdparty/caja-html-sanitizer.js",


This is not related to the resources.json task, right? I would prefer to fix this in a separate PR.

done with 7d41128

matz3 · 2020-08-20T09:59:26Z

lib/processors/uglifier.js

+ *
+ * @type {RegExp}
+ */
+const copyrightCommentsAndBundleCommentPattern = /copyright|\(c\)(?:[0-9]+|\s+[0-9A-za-z])|released under|license|\u00a9|^@ui5-bundle-raw-include |^@ui5-bundle /i;


This is also not directly related to the resources.json, right?
It thought this fixes the dependency analysis, but when reverting the pattern only the uglifier tests are failing.

So maybe some integration tests should be added to cover this scenario?

added integration test with: a3963b9

matz3 · 2020-08-20T10:01:18Z

test/expected/build/application.j/dest-resources-json/changes/coding/MyExtension.js

@@ -0,0 +1 @@
+sap.ui.define([],function(){return{}});


I would also prefer to not add so many test fixtures. We should rather use our fs abstraction to have virtual files within the tests.

separated additional tests into separate PR:
#504

Reduce test fixtures by using the file content string in the tests directly.

* move i18n files to separate folder * revert some.js * put global export logic into newly introduced library.designtime.js file

ensure @ui5-bundle-raw-include comment is also preserved in Builder.js

Not supported with generateStandaloneAppBundle task.

tobiasso85

LGTM

PRs where this process was already used: * SAP/ui5-builder#488 * SAP/ui5-builder#390 JIRA: CPOUI5FOUNDATION-261

codeworrior force-pushed the create-resources-json branch 2 times, most recently from 19ac264 to dcb2252 Compare December 29, 2019 18:18

codeworrior commented Dec 29, 2019

View reviewed changes

tobiasso85 force-pushed the create-resources-json branch from f72c914 to df0fb0b Compare June 22, 2020 13:12

codeworrior commented Jun 25, 2020

View reviewed changes

tobiasso85 force-pushed the create-resources-json branch from 815b826 to ea2ae76 Compare July 29, 2020 12:37

RandomByte reviewed Aug 3, 2020

View reviewed changes

tobiasso85 force-pushed the create-resources-json branch from f2ed108 to f73a050 Compare August 4, 2020 08:10

tobiasso85 changed the title ~~[FEATURE][WIP] Generate resources.json files for libs, components, themes~~ [FEATURE] Generate resources.json files for libs, components, themes Aug 4, 2020

tobiasso85 requested a review from RandomByte August 7, 2020 14:28

RandomByte mentioned this pull request Aug 10, 2020

[FEATURE] Switch XML minifier from pretty-data to minify-xml #488

Merged

3 tasks

RandomByte reviewed Aug 10, 2020

View reviewed changes

tobiasso85 force-pushed the create-resources-json branch from de267bb to 4c15d27 Compare August 11, 2020 06:44

tobiasso85 force-pushed the create-resources-json branch from 4b25ac2 to 6cfe7ab Compare August 11, 2020 14:08

RandomByte reviewed Aug 11, 2020

View reviewed changes

codeworrior commented Aug 11, 2020

View reviewed changes

tobiasso85 force-pushed the create-resources-json branch from e7ade1a to 8b37ebb Compare August 12, 2020 09:01

tobiasso85 added 10 commits August 19, 2020 10:07

version info exclude

393a775

Excluded version info files since they are not part of the resources.json

Rename resourceJson creation method

b416715

Create test for LibraryBuilder

241dbfe

Reduce code size for fixtures

af96c08

Refactored comments

2aad5fe

Reduced test files to the essential

711192c

Reduced test files to the essential

b731011

Used simple copyright statement for test files

86bd41e

Used simple copyright statement for test files

176d76f

Added reason why generateResourcesJson is inactive by default

c8f3a80

tobiasso85 force-pushed the create-resources-json branch from 33223ed to c8f3a80 Compare August 19, 2020 08:07

matz3 reviewed Aug 20, 2020

View reviewed changes

tobiasso85 and others added 8 commits August 20, 2020 14:34

remove unrelated htmlsanitizer fix

7d41128

removed library.n test to reduce js files

c9acba6

removed unnecessary tests

5bbe036

[INTERNAL] Move test files into tests as strings

c7db153

Reduce test fixtures by using the file content string in the tests directly.

[INTERNAL] Reduce test files

423858d

* move i18n files to separate folder * revert some.js * put global export logic into newly introduced library.designtime.js file

Add integration test for uglifier

a3963b9

ensure @ui5-bundle-raw-include comment is also preserved in Builder.js

Add support information

01a5634

Not supported with generateStandaloneAppBundle task.

JSDoc round off

a58c11e

tobiasso85 changed the title ~~[FEATURE] Generate resources.json files for libs, components, themes~~ [FEATURE] Add generateResourcesJson task to generate resources.json which allows to generate resources.json files for libs, components, themes Aug 24, 2020

tobiasso85 changed the title ~~[FEATURE] Add generateResourcesJson task to generate resources.json which allows to generate resources.json files for libs, components, themes~~ [FEATURE] Add generateResourcesJson task Aug 24, 2020

tobiasso85 requested review from matz3 and tobiasso85 August 24, 2020 13:00

tobiasso85 approved these changes Aug 24, 2020

View reviewed changes

matz3 approved these changes Aug 24, 2020

View reviewed changes

tobiasso85 merged commit 021f439 into master Aug 24, 2020

tobiasso85 deleted the create-resources-json branch August 24, 2020 13:17

RandomByte added a commit to SAP/ui5-tooling that referenced this pull request Dec 2, 2020

[INTERNAL] Document the UI5 Tooling benchmarking process

1db338f

PRs where this process was already used: * SAP/ui5-builder#488 * SAP/ui5-builder#390 JIRA: CPOUI5FOUNDATION-261

RandomByte mentioned this pull request Dec 2, 2020

[INTERNAL] Document the UI5 Tooling benchmarking process SAP/ui5-tooling#466

Merged

		@@ -0,0 +1,153 @@
		const ResourceInfo = require("./ResourceInfo");

[FEATURE] Add generateResourcesJson task #390

[FEATURE] Add generateResourcesJson task #390

Conversation

codeworrior commented Dec 29, 2019 • edited by tobiasso85 Loading

coveralls commented Dec 29, 2019 • edited Loading

codeworrior commented Dec 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasso85 Jun 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasso85 Aug 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandomByte commented Aug 7, 2020

RandomByte commented Aug 8, 2020 • edited Loading

RandomByte commented Aug 8, 2020

OpenUI5 sap.m library

OpenUI5 Sample App (+ 6 dependencies)

codeworrior commented Aug 8, 2020

RandomByte commented Aug 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandomByte Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeworrior commented Aug 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandomByte Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandomByte commented Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeworrior commented Dec 29, 2019 •

edited by tobiasso85

Loading

coveralls commented Dec 29, 2019 •

edited

Loading

tobiasso85 Jun 24, 2020 •

edited

Loading

tobiasso85 Aug 4, 2020 •

edited

Loading

RandomByte commented Aug 8, 2020 •

edited

Loading

RandomByte Aug 10, 2020 •

edited

Loading

RandomByte Aug 11, 2020 •

edited

Loading

RandomByte commented Aug 11, 2020 •

edited

Loading