-
Notifications
You must be signed in to change notification settings - Fork 22
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
[FEATURE] Support to only build certain dependencies #442
Conversation
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
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.
Preliminary review. Will finish buildHelper
and tests alter
test/lib/utils/buildHelper.js
Outdated
includedDependencies: ["a"], | ||
excludedDependencies: [] | ||
}; | ||
t.is(alignWithBuilderAPI(false, args.includedDependencies, args.excludedDependencies), true); |
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.
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 🙂
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.
Done
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. |
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.
LGTM
lib/cli/commands/build.js
Outdated
@@ -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", |
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.
I'm missing a hint in the usage texts about the priority of the different filter options.
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.
Done
buildHelper.mergeDependencyLists(includedDependencies, defaultIncludedDependencies, excludedDependencies); | ||
|
||
const buildAll = buildHelper.alignWithBuilderApi(argv.all, includedDependencies, excludedDependencies); | ||
|
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.
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)
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.
I have changed the implementation. The new code has a lot more comments for describing priority.
lib/utils/buildHelper.js
Outdated
} | ||
dependencyList.forEach((dep) => { | ||
if ( | ||
dep instanceof RegExp && !isRegExpInList(targetDependencyList, dep) && !isRegExpInList(excludeList, dep) || |
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.
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 abovebelow, 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.
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.
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.
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 described in #442 (comment)
This priority issue and comparison is improved now. RegExps are no longer considered separately.
await builder.build({ | ||
tree: tree, | ||
destPath: argv.dest, | ||
cleanDest: argv["clean-dest"], | ||
buildDependencies: argv.all, | ||
buildDependencies: buildAll, |
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.
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?
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.
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.
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:
Hardware: MacBook Pro 2018 - i9 2,9GHz/32GB
|
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.
I'm pretty happy with the new createDependencyLists
. Curious what @codeworrior thinks
I'm fine with this PR but I would like to give @codeworrior the chance for another review. |
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.
LGTM, just a few comments regarding documentation.
lib/utils/buildHelper.js
Outdated
* 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' |
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.
Generics (<TypeParam, TypeParam...>), can only be used with the upper case Object, not with the primitive "object".
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.
Done
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.
I just had to revert to the lowercase style because the UI5 Tooling eslint rules doesn't allow the uppercase notation.
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.
Then please validate it locally: npm run jsdoc
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.
Sorry, then the rule is wrong, disable it 😄
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.
I've updated my last comment with a smiley to indicate that I was not 100% serious, only 95%.
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.
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.
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.
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.
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.
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
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.
Or add a single line disable. I'll leave that to @larskissel
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.
I think we are good to go
Please use "Squash and merge" to have this as a single commit in master |
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