[FIX] generateFlexChangesBundle: Fix minUI5Version check for UI5 v1.100.0+ #706
Conversation
| let hasFlexBundleVersion = false; | ||
| if (parseFloat(version) >= 1.73) { | ||
| if (versionArray.length >= 2 && parseFloat(versionArray[0]) >= 1 && parseFloat(versionArray[1]) >= 73) { |
There was a problem hiding this comment.
Please use the semver package to compare versions. It's already a dependency of this project, so you just need to require it.
| let hasFlexBundleVersion = false; | ||
| if (versionArray.length >= 2 && parseFloat(versionArray[0]) >= 1 && parseFloat(versionArray[1]) >= 73) { | ||
| if (version.major >= MIN_UI5_VERION_1_73.major && version.minor >= MIN_UI5_VERION_1_73.minor) { |
There was a problem hiding this comment.
Could compare method be used?
There was a problem hiding this comment.
Ah I viewed an old version
matz3
changed the title
|
@thuyboehm could you please sign the CLA via https://cla-assistant.io/SAP/ui5-builder?pullRequest=706 ? |
| let hasFlexBundleVersion = false; | ||
| if (parseFloat(version) >= 1.73) { | ||
| if (version.compare(MIN_UI5_VERION_1_73) >= 0) { |
There was a problem hiding this comment.
Just my two cents: This is a pretty odd way of using the semver API, that I haven't seen so far and also couldn't find in the documentation at https://github.com/npm/node-semver#readme
IMHO semver.compare(version, "1.73.0") >= 0 would be sufficient and does not involve the creation of Version instances.
But I guess your solution works too and since it's your code we are totally fine with merging this as is. Please let us know soon in case you want to change this. Otherwise we will proceed with releasing this PR.
There was a problem hiding this comment.
Edit: I meant semver.compare, not version.compare
There was a problem hiding this comment.
Isn't it necessary to semver.coerce the version from the manifest first? I recently worked with semver for the first time and was puzzled that it doesn't do this automatically, at least not with the convenience comparison methods (semver.lt etc.).
There was a problem hiding this comment.
Yes, it is. As the manifest.json schema doesn't validate the "minUI5Version" properly (just type "string"), it is likely that the patch version is missing in some cases (e.g. 1.100). This should also be covered in the tests.
There was a problem hiding this comment.
Well, even our manifestCreator just uses major.minor without the patch version.
There was a problem hiding this comment.
...which totally makes sense to me as the patch version doesn't play a role wrt. APIs. It's just that semver doesn't handle such short versions with a call to semver.coerce.
|
Maybe some test cases for the "major.minor" version would be good to have, right? |
|
It is ok now? |
|
Released via @ui5/builder v2.11.4 / @ui5/cli v2.14.6 |
The minUI5Version check is needed for generate a flexibility-bundle or a change-bundle. The check is now separated for the major and minor version number.