Skip to content
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.

Sign up for GitHub

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

(iOS) Added Semver to version comparison checks. #695

Merged
merged 12 commits into from
Oct 23, 2019
Merged

(iOS) Added Semver to version comparison checks. #695

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
81f6b1c
Allowed extra variation in version comparison
mattmilan-dev Oct 20, 2019
bfc92d5
Update versions.js
mattmilan-dev Oct 20, 2019
1d71673
Add semver support to version comparison
Oct 21, 2019
1603bb9
Bug fix (forgot to remove !)
mattmilan-dev Oct 21, 2019
f022350
Equal versions perform check favouring non-prerelease
mattmilan-dev Oct 22, 2019
712f803
Update unit test
Oct 22, 2019
dcec805
Merge remote-tracking branch 'origin/master'
Oct 22, 2019
4244ea3
Removes EOL whitespace for ES Lint
Oct 22, 2019
d64f9ad
Remove package-lock.json which sneaked itself in
raphinesse Oct 22, 2019
ef20545
Update method and unit test
Oct 23, 2019
20429ea
Fix versions.compareVersions to be more forgiving
raphinesse Oct 23, 2019
5742b74
Add unit tests for versions.compareVersions
raphinesse Oct 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 15 additions & 22 deletions bin/templates/scripts/cordova/lib/versions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

var child_process = require('child_process');
var Q = require('q');
var semver = require('semver');

exports.get_apple_ios_version = function () {
var d = Q.defer();
Expand Down Expand Up @@ -167,28 +168,20 @@ exports.get_tool_version = function (toolName) {
* positive otherwise and 0 if versions are equal.
*/
exports.compareVersions = function (version1, version2) {
function parseVer (version) {
return version.split('.').map(function (value) {
// try to convert version segment to Number
var parsed = Number(value);
// Number constructor is strict enough and will return NaN
// if conversion fails. In this case we won't be able to compare versions properly
if (isNaN(parsed)) {
throw 'Version should contain only numbers and dots';
}
return parsed;
});
// coerce and validate versions
var cleanV1 = semver.valid(semver.coerce(version1));
var cleanV2 = semver.valid(semver.coerce(version2));

// throw exception in the event one or both versions cannot be validated
if (cleanV1 === null || cleanV2 === null) {
throw 'Version should be in valid semver syntax. See: https://semver.org/';
}
var parsedVer1 = parseVer(version1);
var parsedVer2 = parseVer(version2);

// Compare corresponding segments of each version
for (var i = 0; i < Math.max(parsedVer1.length, parsedVer2.length); i++) {
// if segment is not specified, assume that it is 0
// E.g. 3.1 is equal to 3.1.0
var ret = (parsedVer1[i] || 0) - (parsedVer2[i] || 0);
// if segments are not equal, we're finished
if (ret !== 0) return ret;

// if versions are equivalent, return 0;
if (cleanV1 === cleanV2) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

semver.coerce drops labels.

When cleanV1 and cleanV2 is equal, do you think it's worth checking the original version variables and determine if one has a prerelease label, then return the appropriate integer?

e.g.

var version1 = "1.0.0";
var version2 = "1.0.0-beta.0";

var cleanV1 = semver.valid(semver.coerce(version1));
var cleanV2 = semver.valid(semver.coerce(version2));

if (cleanV1 === cleanV2) {
    if (version1.indexOf('-') > -1 && version2.indexOf('-') === -1) {
        // version1 is prerelease, favour version2 instead.
    }
    else if (version2.indexOf('-') > -1 && version1.indexOf('-') === -1) {
        //version 2 is prelease, favour version 1
    }
    else {
        return 0;
    }
}

Copy link
Contributor Author

@mattmilan-dev mattmilan-dev Oct 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes a lot of sense to do that actually. I've used .includes as a formality to make the code easier to read, as the performance loss is minimal in comparison to indexOf. However, if we need backwards compatibility with < IE 11 (As it's a CLI tool i'm assuming this isn't necessary), i'll go with the indexOf instead!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use .includes, as long as it's supported in Node 6. If not, then this patch needs to wait until we drop support for node 6, which will likely come in next major release.

Copy link
Contributor

@raphinesse raphinesse Oct 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our JSDocs say "Compares two semver-notated version strings". So in my opinion, this whole function could (and should) be implemented as follows:

exports.compareVersions = semver.compare

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize this but semver actually does handling comparing prerelease labels, so my suggestion is just doing the work over again.

We also don't need to use coerce.

semver.valid("1.0.0.0"); // null
semver.valid("1.0.0"): // "1.0.0"
semver.valid("1.0.0-beta.0"); // "1.0.0-beta.0"

Exporting semver.compare directly will change what compareVersions will return as an error, if it does receive an invalid semver, which will need to refactored in the unit tests but I think that's fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the expectation in the unit test will have to be adapted for the new error

return 0;
}
return 0;

// alternatively return positive/negative number
return semver.gt(cleanV1, cleanV2) ? 1 : -1;
};
Loading