[FEATURE] Support to only build certain dependencies #442

larskissel · 2021-07-09T13:34:33Z

Projects can be built with a selection of dependencies that
have to be included into the build result. The following CLI
parameters can be used flexibly to configure the selection
of the dependencies to be built:
--include-dependency,
--include-dependency-regexp,
--include-dependency-tree,
--exclude-dependency,
--exclude-dependency-regexp,
--exclude-dependency-tree

JIRA: CPOUI5FOUNDATION-208
Resolves: SAP/ui5-tooling#494

Projects can be built with a selection of dependencies that have to be included into the build result. The following CLI parameters can be used flexibly to configure the selection of the dependencies to be built: --include-dependency, --include-dependency-regexp, --include-dependency-tree, --exclude-dependency, --exclude-dependency-regexp, --exclude-dependency-tree JIRA: CPOUI5FOUNDATION-208

coveralls · 2021-07-09T13:35:33Z

Coverage increased (+0.2%) to 95.954% when pulling e69b951 on buildWithSelectedDependencies into 3ddb222 on master.

RandomByte

Preliminary review. Will finish buildHelper and tests alter

lib/cli/commands/build.js

lib/utils/buildHelper.js

lib/cli/commands/build.js

lib/utils/buildHelper.js

test/lib/utils/buildHelper.js

RandomByte · 2021-07-12T17:03:39Z

test/lib/utils/buildHelper.js

+		includedDependencies: ["a"],
+		excludedDependencies: []
+	};
+	t.is(alignWithBuilderAPI(false, args.includedDependencies, args.excludedDependencies), true);


Please pass some messages as the third argument to these assertions to describe them. Otherwise you'll just see error messages like

"alignWithBuilderAPI: * is added to excludedDependencies" failed because false it not true

Which is not very helpful, especially when doing some major refactoring and you are confronted with hundreds of other, similarly unclear, error messages 🙂

RandomByte · 2021-07-12T17:10:08Z

Once you have added even more recursive tree traversals I think it's reasonable to do some basic performance tests on a project with many dependencies (like the OpenUI5 testsuite). Just to make sure we don't add too much time to the build process.

We documented the benchmarking process for the openui5-sample-app here: https://sap.github.io/ui5-tooling/pages/Troubleshooting/#benchmarking-ui5-tooling. You could either add some more dependencies to the sample app or use the OpenUI5 testsuite. We can also discuss this briefly in the team.

KlattG

LGTM

lib/cli/commands/build.js

codeworrior · 2021-07-14T06:38:16Z

lib/cli/commands/build.js

@@ -37,6 +38,32 @@ build.builder = function(cli) {
 			default: false,
 			type: "boolean"
 		})
+		.option("include-dependency", {
+			describe: "A list of dependencies to be included into the build process",


I'm missing a hint in the usage texts about the priority of the different filter options.

codeworrior · 2021-07-14T06:40:16Z

lib/cli/commands/build.js

+	buildHelper.mergeDependencyLists(includedDependencies, defaultIncludedDependencies, excludedDependencies);
+
+	const buildAll = buildHelper.alignWithBuilderApi(argv.all, includedDependencies, excludedDependencies);
+


It's hard to understand from the code what the overall logic (idea) regarding priorities is. Please add a comment describing exactly this (what filter has priority over what other filter)

I have changed the implementation. The new code has a lot more comments for describing priority.

lib/cli/commands/build.js

lib/utils/buildHelper.js

codeworrior · 2021-07-14T07:23:27Z

lib/utils/buildHelper.js

+	}
+	dependencyList.forEach((dep) => {
+		if (
+			dep instanceof RegExp && !isRegExpInList(targetDependencyList, dep) && !isRegExpInList(excludeList, dep) ||


the check !isRegExpInList(excludeList, dep)) looks tricky to me. I understand that it is balanced with the !isInList(...)check in the next line. But while the !isInList(...)check gives an exact result, this one here is only an approximation. There will be cases where an includeRegExp (/^ab/) is totally overruled by an excludeRegExp (e.g. /^a/), but the check can't detect this.

I guess the answer will also answer my question ~~above~~below, why the builder still requires include and exclude options and why a preprocessing of the tree is not enough. There must be a later phase that handles the regexp correctly. But if so, why not totally leave it to that phase?

Or the other way around: why is it not possible to calculate the final list of "projects to be built" and give that to the builder? Looks strange to me that the filtering has to be split across layers.

We use this function on three occasions:

Merging the resolved include-trees (which do not contain regular expressions) into the includes list

Merging the resolved exclude-trees (which do not contain regular expressions) into the excludes list

Merging the includes (string, regex and resolved trees) from the ui5.yaml configuration into the includes list

I think if we find a different solution for merging 3 into the list of includes, we can simplify this function quite a bit, since dependencyList can then never be a regular expression.

As for merging the ui5.yaml defaults into the list, we could collet the regular expression includes in a separate variable which is always pushed onto the list and only use mergeDependencyLists for the "string" includes and the trees.

As described in #442 (comment)
This priority issue and comparison is improved now. RegExps are no longer considered separately.

codeworrior · 2021-07-14T07:24:58Z

lib/cli/commands/build.js

 	await builder.build({
 		tree: tree,
 		destPath: argv.dest,
 		cleanDest: argv["clean-dest"],
-		buildDependencies: argv.all,
+		buildDependencies: buildAll,


just out of curiosity: why does the builder still require include/exclude? We're already traversing the tree, looking at the new filtering options. Why can't we resolve the filters in the CLI layer and just give a list of projects (includes maybe) to the builder?

why does the builder still require include/exclude?

For consumers like Kristian, who use the Node.js API of the builder directly.

I would try to keep the CLI layer as simple as possible and just pass the includes/excludes to the builder mostly untouched.

larskissel · 2021-07-16T11:03:41Z

I just did a benchmark of the current implementation. For a better comparability I have disabled the actual build process - so, it describes the processing time only within the ui5-cli.

Benchmarked application: https://github.com/SAP/openui5-sample-app

Enhanced application dependencies:

ui5 use sapui5
ui5 add sap.apf sap.ca.scfld.md sap.ca.ui sap.chart sap.collaboration sap.f sap.fe.common sap.fe.core sap.fe.macros sap.fe.navigation sap.fe.placeholder sap.fe.plugins sap.fe.templates sap.fe.test sap.feedback.ui sap.gantt sap.landvisz sap.m sap.makit sap.me sap.ndc sap.ovp sap.rules.ui sap.suite.ui.commons sap.suite.ui.generic.template sap.suite.ui.microchart sap.tnt sap.ui.codeeditor sap.ui.commons sap.ui.comp sap.ui.core sap.ui.dt sap.ui.export sap.ui.fl sap.ui.generic.app sap.ui.generic.template sap.ui.integration sap.ui.layout sap.ui.mdc sap.ui.richtexteditor sap.ui.rta sap.ui.suite sap.ui.support sap.ui.table sap.ui.testrecorder sap.ui.unified sap.ui.ux3 sap.ui.vbm sap.ui.vk sap.ui.vtm sap.uiext.inbox sap.ushell sap.ushell_abap sap.uxap sap.viz sap.webanalytics.core sap.zen.commons sap.zen.crosstab sap.zen.dsh themelib_sap_belize themelib_sap_bluecrystal themelib_sap_fiori_3

Hardware: MacBook Pro 2018 - i9 2,9GHz/32GB

ui5-cli Ref	Command	Mean [s]	Min [s]	Max [s]	Relative
master (`4bd0ad8`)	`ui5 build`	1.323 ± 0.028	1.283	1.358	1.00
buildWithSelectedDependencies (`a187584`)	`ui5 build --include-dependency-regexp="sap\.ui\.\w+" --exclude-dependency-tree="sap.m"`	1.339 ± 0.032	1.298	1.374	1.00

RandomByte

I'm pretty happy with the new createDependencyLists. Curious what @codeworrior thinks

lib/cli/commands/build.js

lib/utils/buildHelper.js

RandomByte · 2021-07-19T17:37:55Z

I'm fine with this PR but I would like to give @codeworrior the chance for another review.

lib/cli/commands/build.js

…ndency"

codeworrior

LGTM, just a few comments regarding documentation.

codeworrior · 2021-07-23T09:04:04Z

lib/utils/buildHelper.js

+ *   for build settings
+ * @param {Array.<string>} parameters.defaultIncludeDependencyTree Same as 'includeDependencyTree' parameter; used for
+ *   build settings
+ * @returns {object<string, string[]>} An object containing the 'includedDependencies' and 'excludedDependencies'


Generics (<TypeParam, TypeParam...>), can only be used with the upper case Object, not with the primitive "object".

I just had to revert to the lowercase style because the UI5 Tooling eslint rules doesn't allow the uppercase notation.

Then please validate it locally: npm run jsdoc

Sorry, then the rule is wrong, disable it 😄

I've updated my last comment with a smiley to indicate that I was not 100% serious, only 95%.

I've updated my last comment with a smiley to indicate that I was not 100% serious, only 95%.

Thank you! It left me quite puzzled 😵‍💫

In the UI5 Tooling JSDoc it seems that both work. The difference in the HTML is simply:

object.<string, Array.>

vs

Object.<string, Array.>

In this case you're right that we indeed return an Object but it's also an object. From the rule:

To make things more confusing, there are also object literals and object Objects. But object literals are still static Objects and object Objects are instantiated Objects. So an object primitive is still an object Object.

Rule documentation IMO is only a second level of truth. I refer to https://jsdoc.app/tags-type.html and https://github.com/google/closure-compiler/wiki/Types-in-the-Closure-Type-System .

But you're free to ignore me. I didn't "request changes", I only commented.

But you're free to ignore me.

Will do 😉

Let's keep it as-is for now. IMHO the documentation is good enough 🥉 . Changing the ESLint configuration across all projects is just too much work to make this (private) documentation perfect.

Related discussion in eslint-plugin-jsdoc: gajus/eslint-plugin-jsdoc#709

Or add a single line disable. I'll leave that to @larskissel

lib/utils/buildHelper.js

RandomByte

I think we are good to go

RandomByte · 2021-07-23T11:24:08Z

Please use "Squash and merge" to have this as a single commit in master

larskissel requested review from matz3 and RandomByte July 12, 2021 08:19

RandomByte requested a review from KlattG July 12, 2021 13:43

RandomByte requested changes Jul 12, 2021

View reviewed changes

KlattG suggested changes Jul 12, 2021

View reviewed changes

RandomByte requested changes Jul 12, 2021

View reviewed changes

larskissel added 2 commits July 13, 2021 17:51

[INTERNAL] Apply suggestions from code review

63f30e5

[INTERNAL] Fix ESlint issue

119fd55

larskissel requested review from RandomByte and KlattG July 13, 2021 16:12

KlattG previously approved these changes Jul 14, 2021

View reviewed changes

codeworrior reviewed Jul 14, 2021

View reviewed changes

[INTERNAL] Improve creation of dependency lists, enhance documentation

6add4ea

larskissel dismissed KlattG’s stale review via 6add4ea July 16, 2021 07:44

[INTERNAL] Fix broken test

a187584

RandomByte requested changes Jul 16, 2021

View reviewed changes

[INTERNAL] Apply suggestions from code review

7ac5b84

RandomByte mentioned this pull request Jul 19, 2021

Option to only build certain dependencies SAP/ui5-tooling#494

Closed

6 tasks

KlattG suggested changes Jul 20, 2021

View reviewed changes

lib/cli/commands/build.js Outdated Show resolved Hide resolved

[INTERNAL] Improve build command description for option "include-depe…

7fe851f

…ndency"

codeworrior reviewed Jul 23, 2021

View reviewed changes

larskissel added 2 commits July 23, 2021 12:01

[INTERNAL] Improve JSDoc param types

692d93b

[INTERNAL] Fix JSDoc return types

e69b951

RandomByte approved these changes Jul 23, 2021

View reviewed changes

larskissel merged commit 5f941f1 into master Jul 23, 2021

larskissel deleted the buildWithSelectedDependencies branch July 23, 2021 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] Support to only build certain dependencies #442

[FEATURE] Support to only build certain dependencies #442

larskissel commented Jul 9, 2021 •

edited by RandomByte

coveralls commented Jul 9, 2021 •

edited

RandomByte left a comment

RandomByte Jul 12, 2021

larskissel Jul 13, 2021

RandomByte commented Jul 12, 2021

KlattG left a comment

codeworrior Jul 14, 2021

larskissel Jul 16, 2021

codeworrior Jul 14, 2021 •

edited

larskissel Jul 16, 2021

codeworrior Jul 14, 2021 •

edited

RandomByte Jul 14, 2021

larskissel Jul 16, 2021

codeworrior Jul 14, 2021

RandomByte Jul 14, 2021 •

edited

larskissel commented Jul 16, 2021

RandomByte left a comment

RandomByte commented Jul 19, 2021

codeworrior left a comment

codeworrior Jul 23, 2021

larskissel Jul 23, 2021

larskissel Jul 23, 2021

RandomByte Jul 23, 2021

codeworrior Jul 23, 2021 •

edited

codeworrior Jul 23, 2021

RandomByte Jul 23, 2021 •

edited

codeworrior Jul 23, 2021

RandomByte Jul 23, 2021

RandomByte Jul 23, 2021

RandomByte left a comment

RandomByte commented Jul 23, 2021

		buildHelper.mergeDependencyLists(includedDependencies, defaultIncludedDependencies, excludedDependencies);

		const buildAll = buildHelper.alignWithBuilderApi(argv.all, includedDependencies, excludedDependencies);

[FEATURE] Support to only build certain dependencies #442

[FEATURE] Support to only build certain dependencies #442

Conversation

larskissel commented Jul 9, 2021 • edited by RandomByte

coveralls commented Jul 9, 2021 • edited

RandomByte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandomByte commented Jul 12, 2021

KlattG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeworrior Jul 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeworrior Jul 14, 2021 • edited

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 Jul 14, 2021 • edited

Choose a reason for hiding this comment

larskissel commented Jul 16, 2021

RandomByte left a comment

Choose a reason for hiding this comment

RandomByte commented Jul 19, 2021

codeworrior left a 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

codeworrior Jul 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandomByte Jul 23, 2021 • edited

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 left a comment

Choose a reason for hiding this comment

RandomByte commented Jul 23, 2021

larskissel commented Jul 9, 2021 •

edited by RandomByte

coveralls commented Jul 9, 2021 •

edited

codeworrior Jul 14, 2021 •

edited

codeworrior Jul 14, 2021 •

edited

RandomByte Jul 14, 2021 •

edited

codeworrior Jul 23, 2021 •

edited

RandomByte Jul 23, 2021 •

edited