sem-ver implemented by msohailhussain · Pull Request #566 · optimizely/javascript-sdk

msohailhussain · 2020-09-15T05:18:53Z

Summary

Semantic version added
Greater than or equal (ge) and less than or equal (le) evaluator added

Test plan

All relevant test cases added

coveralls · 2020-09-15T05:22:48Z

Coverage decreased (-0.3%) to 96.424% when pulling 8dc6368 on uzair/semver-ts into 28260a2 on master.

mnoman09

Overall looks good. Added some comments.

packages/optimizely-sdk/lib/utils/semantic_version/index.ts

packages/optimizely-sdk/lib/core/custom_attribute_condition_evaluator/index.tests.js

mnoman09 · 2020-09-15T20:56:42Z

packages/optimizely-sdk/lib/core/custom_attribute_condition_evaluator/index.ts

+  const userValueType = typeof userValue;
+  const conditionValue = condition.value;
+
+  if (conditionValue === null || !isSafeInteger(conditionValue)) {


instead of repeating all these checks in every function put it in one function and use it from there see C# for reference. the only evaluation logic should be kept inside every evaluator.

Consistency with previous written evaluators like greaterThanEvaluator, lessThanEvaluator etc will be lost.

I agree with @mnoman09. If there is repeated logic for checking user inputs for numeric comparison conditions, let's use a shared helper function. If the logic is not exactly the same or it is awkward to share, don't worry about it.

mnoman09 · 2020-09-15T21:05:37Z

packages/optimizely-sdk/lib/utils/semantic_version/index.tests.js

+          ['2.9.9-beta', '2.9.9-beta'],
+          ['2.1', '2.1.0'],
+          ['2', '2.12'],
+          ['2.9', '2.9.1']


Add this test where targetSV = 2.1.3 and userver= 2.1.3+build and it should return 0 see this test

2.1.3 > 2.1.3+build

No this is a corner case that we covered in all other sdks. you can see java link I shared above.

mnoman09

lgtm

mjc1283 · 2020-09-16T20:09:18Z

packages/optimizely-sdk/lib/core/custom_attribute_condition_evaluator/index.ts

+  const userValueType = typeof userValue;
+  const conditionValue = condition.value;
+
+  if (conditionValue === null || !isSafeInteger(conditionValue)) {


I agree with @mnoman09. If there is repeated logic for checking user inputs for numeric comparison conditions, let's use a shared helper function. If the logic is not exactly the same or it is awkward to share, don't worry about it.

mjc1283 · 2020-09-16T20:17:59Z

packages/optimizely-sdk/lib/utils/enums/index.ts

+export const VERSION_TYPE = {
+  IS_PRE_RELEASE: '-',
+  IS_BUILD: '+'
+}


TypeScript has enum and const enum declarations. Some can be compiled into constant values leading to smaller code size. Please check the documentation for enums. Let's not use an object with long property names unless necessary, to avoid bloating the code.

mjc1283 · 2020-09-16T20:19:21Z

packages/optimizely-sdk/lib/utils/semantic_version/index.ts

+
+  }
+
+  export function compareVersion(conditionsVersion: string, userProvidedVersion: string): number | null {


Please add a documentation comment

mjc1283 · 2020-09-16T20:20:15Z

packages/optimizely-sdk/lib/utils/semantic_version/index.ts

+    if (!userVersionParts || !conditionsVersionParts)
+      return null;


Style preference is to always use brackets with if statements.

mjc1283 · 2020-09-16T20:22:15Z

packages/optimizely-sdk/lib/utils/semantic_version/index.ts

+      if (userVersionPartsLen <= idx)
+          return isPreReleaseInconditionsVersion || isBuildInconditionsVersion ? 1 : -1
+      else if (!isNumber(userVersionParts[idx])) {
+        if (userVersionParts[idx] < conditionsVersionParts[idx]) {
+          return isPreReleaseInconditionsVersion && !isPreReleaseInuserProvidedVersion ? 1 : -1;
+        }
+        else if (userVersionParts[idx] > conditionsVersionParts[idx]) {
+          return !isPreReleaseInconditionsVersion && isPreReleaseInuserProvidedVersion ? -1 : 1;
+        }
+      }
+      else {
+        const userVersionPart = parseInt(userVersionParts[idx])
+        const conditionsVersionPart = parseInt(conditionsVersionParts[idx])
+        if (userVersionPart > conditionsVersionPart)
+            return 1;
+        else if (userVersionPart < conditionsVersionPart)
+            return -1;
+      }


Please try to simplify the control flow, there are a lot of nested conditionals.

mjc1283 · 2020-09-16T20:25:36Z

packages/optimizely-sdk/lib/utils/semantic_version/index.ts

+  *                     
+  */
+  function isNumber(content: string): boolean {
+    return content.match(/^[0-9]+$/) != null ? true : false;


Is this equivalent?

Suggested change

return content.match(/^[0-9]+$/) != null ? true : false;

return /^\d+$/.test(content);

(see: Regular Expression docs for JS)

Also: I suggest declaring regular expression once higher up in module scope, then reference here (DIGIT_STRING_REGEXP.test(content)).

mjc1283 · 2020-09-16T20:27:57Z

packages/optimizely-sdk/lib/utils/semantic_version/index.ts

+   *                     
+   */
+  function hasWhiteSpaces(version: string): boolean {
+    return version.includes(' ');


What about other space characters (tab, newline, etc.)? Should we use space regexp character class?

Suggested change

return version.includes(' ');

return /\s/.test(version);

Minor concern: String.prototype.includes is not supported in very old JS environments.

Is there any performance difference between String.prototype.includes and regular expression test method?

mjc1283 · 2020-09-16T20:31:00Z

packages/optimizely-sdk/lib/utils/semantic_version/index.ts

+    //check for pre release e.g. 1.0.0-alpha where 'alpha' is a pre release
+    //otherwise check for build e.g. 1.0.0+001 where 001 is a build metadata
+    if (isPreReleaseVersion(version)) {
+      targetPrefix = version.substring(0, version.indexOf(VERSION_TYPE.IS_PRE_RELEASE));


IS_PRE_RELEASE sounds like a boolean. I prefer PRE_RELEASE_VERSION_DELIMITER.

mjc1283 · 2020-09-16T20:31:19Z

packages/optimizely-sdk/lib/utils/semantic_version/index.ts

+      targetSuffix = version.substring(version.indexOf(VERSION_TYPE.IS_PRE_RELEASE) + 1);
+    }
+    else if (isBuildVersion(version)) {
+        targetPrefix = version.substring(0, version.indexOf(VERSION_TYPE.IS_BUILD));


BUILD_VERSION_DELIMITER

mjc1283 · 2020-09-16T20:36:51Z

packages/optimizely-sdk/lib/utils/semantic_version/index.ts

+   * @return {boolean}  The array of version split into smaller parts i.e major, minor, patch etc
+   *                    null if given version is in invalid format 


Would consumer code be clearer if we had an interface for semantic version components?

interface SemanticVersionComponents { major: number; minor: number; patch: number; preRelease?: number build?: number }

Then:

function splitVersion(version: string): SemanticVersionComponents | null {

sem-ver implemented

95d3716

optimizely-codereview-bot assigned msohailhussain Sep 15, 2020

mnoman09 suggested changes Sep 15, 2020

View reviewed changes

uzair-folio3 added 3 commits September 16, 2020 07:03

evaluator name fixed

ecad620

test cases added

d56de7d

Update index.ts

8dc6368

mnoman09 approved these changes Sep 16, 2020

View reviewed changes

msohailhussain removed their assignment Sep 16, 2020

optimizely-codereview-bot requested a review from ozayr-zaviar September 16, 2020 18:30

msohailhussain removed the request for review from ozayr-zaviar September 16, 2020 18:30

ozayr-zaviar closed this Sep 16, 2020

mjc1283 reviewed Sep 16, 2020

View reviewed changes

msohailhussain mentioned this pull request Sep 17, 2020

feat: semver implementation #571

Merged

raju-opti deleted the uzair/semver-ts branch October 4, 2023 13:52


		}

		export function compareVersion(conditionsVersion: string, userProvidedVersion: string): number \| null {

		if (!userVersionParts \|\| !conditionsVersionParts)
		return null;

	return content.match(/^[0-9]+$/) != null ? true : false;
	return /^\d+$/.test(content);

		* @return {boolean} The array of version split into smaller parts i.e major, minor, patch etc
		* null if given version is in invalid format

Conversation

msohailhussain commented Sep 15, 2020 • edited by ozayr-zaviar Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

coveralls commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mnoman09 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ozayr-zaviar Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnoman09 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

msohailhussain commented Sep 15, 2020 •

edited by ozayr-zaviar

Loading

coveralls commented Sep 15, 2020 •

edited

Loading

ozayr-zaviar Sep 16, 2020 •

edited

Loading