From 6cd32ee5faab9e6505416271abc413d9fd38771c Mon Sep 17 00:00:00 2001
From: alxndrsn <alxndrsn>
Date: Thu, 14 Nov 2024 05:20:09 +0000
Subject: [PATCH 1/3] Option: fix should.js .eql() behaviour

Making Option.value a private field broke should(Option).eql() behaviour.

See: #1287 / 5ef4c721664b344463ae7520ac3b1fa24c879402

Closes #1296
---
 lib/util/option.js       | 27 +++++++++++++++++----------
 test/unit/util/option.js | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 10 deletions(-)

diff --git a/lib/util/option.js b/lib/util/option.js
index 400b28d6f..a350e2479 100644
--- a/lib/util/option.js
+++ b/lib/util/option.js
@@ -38,28 +38,35 @@ class Option {
 }
 
 class Some extends Option {
-  #value;
+  // Should.js uses `for ... in` to compare objects, so while should.js is in
+  // use an Option's value property must be "enumerable".
+  //
+  // See: https://github.com/shouldjs/equal/blob/master/index.js
+  // See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Enumerability_and_ownership_of_properties
+  //
+  // TODO revert from __please_use_get to #value when should.js is removed.
+  //#value;
 
   /* istanbul ignore next */
   constructor(value) {
     super();
-    this.#value = value;
+    this.__please_use_get = value;
   }
 
-  get() { return this.#value; }
+  get() { return this.__please_use_get; }
 
-  map(fn) { return Option.of(fn(this.#value)); }
-  filter(predicate) { return predicate(this.#value) === true ? this : none; } // eslint-disable-line no-use-before-define
+  map(fn) { return Option.of(fn(this.__please_use_get)); }
+  filter(predicate) { return predicate(this.__please_use_get) === true ? this : none; } // eslint-disable-line no-use-before-define
 
-  orNull() { return this.#value; }
-  orElse() { return this.#value; }
-  orElseGet() { return this.#value; }
-  orThrow() { return this.#value; }
+  orNull() { return this.__please_use_get; }
+  orElse() { return this.__please_use_get; }
+  orElseGet() { return this.__please_use_get; }
+  orThrow() { return this.__please_use_get; }
 
   isDefined() { return true; }
   isEmpty() { return false; }
 
-  ifDefined(consumer) { consumer(this.#value); }
+  ifDefined(consumer) { consumer(this.__please_use_get); }
 
   // Used by ramda for comparing objects.
   equals(that) {
diff --git a/test/unit/util/option.js b/test/unit/util/option.js
index 392e43de5..65ecdb0cd 100644
--- a/test/unit/util/option.js
+++ b/test/unit/util/option.js
@@ -149,4 +149,40 @@ describe('(libs/FP) Option type', () => {
       });
     });
   });
+
+  describe('should.js interactions', () => {
+
+    // N.B. should.equal() is different from should.eql():
+    //
+    // * .eql(): check equality using ===
+    // * .eql(): check equality using "should-equal" module
+    //
+    // See: https://www.npmjs.com/package/should-equal
+
+    describe('should.eql()', () => {
+      [
+        true,
+        false,
+        0,
+        1,
+        '',
+        'non-empty string',
+      ].forEach(val => {
+        it(`should recognise two Options of '${val}' to be equal`, () => {
+          Option.of(val).should.eql(Option.of(val));
+        });
+      });
+
+      [
+        [ 0, 1 ],
+        [ 0, false ],
+        [ 0, '' ],
+        [ false, '' ],
+      ].forEach((a, b) => {
+        it(`should not recognise Options of '${a}' and '${b}' as equal`, () => {
+          Option.of(a).should.not.eql(Option.of(b));
+        });
+      });
+    });
+  });
 });

From e37264eb1681edda4e63af2708f637cc9dcb603c Mon Sep 17 00:00:00 2001
From: alxndrsn <alxndrsn>
Date: Wed, 20 Nov 2024 05:39:22 +0000
Subject: [PATCH 2/3] use _value; add more tests

---
 lib/util/option.js       | 27 +++++++++------------------
 test/unit/util/option.js | 31 ++++++++++++++++++++++++++-----
 2 files changed, 35 insertions(+), 23 deletions(-)

diff --git a/lib/util/option.js b/lib/util/option.js
index a350e2479..3c9117687 100644
--- a/lib/util/option.js
+++ b/lib/util/option.js
@@ -38,35 +38,26 @@ class Option {
 }
 
 class Some extends Option {
-  // Should.js uses `for ... in` to compare objects, so while should.js is in
-  // use an Option's value property must be "enumerable".
-  //
-  // See: https://github.com/shouldjs/equal/blob/master/index.js
-  // See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Enumerability_and_ownership_of_properties
-  //
-  // TODO revert from __please_use_get to #value when should.js is removed.
-  //#value;
-
   /* istanbul ignore next */
   constructor(value) {
     super();
-    this.__please_use_get = value;
+    this._value = value;
   }
 
-  get() { return this.__please_use_get; }
+  get() { return this._value; }
 
-  map(fn) { return Option.of(fn(this.__please_use_get)); }
-  filter(predicate) { return predicate(this.__please_use_get) === true ? this : none; } // eslint-disable-line no-use-before-define
+  map(fn) { return Option.of(fn(this._value)); }
+  filter(predicate) { return predicate(this._value) === true ? this : none; } // eslint-disable-line no-use-before-define
 
-  orNull() { return this.__please_use_get; }
-  orElse() { return this.__please_use_get; }
-  orElseGet() { return this.__please_use_get; }
-  orThrow() { return this.__please_use_get; }
+  orNull() { return this._value; }
+  orElse() { return this._value; }
+  orElseGet() { return this._value; }
+  orThrow() { return this._value; }
 
   isDefined() { return true; }
   isEmpty() { return false; }
 
-  ifDefined(consumer) { consumer(this.__please_use_get); }
+  ifDefined(consumer) { consumer(this._value); }
 
   // Used by ramda for comparing objects.
   equals(that) {
diff --git a/test/unit/util/option.js b/test/unit/util/option.js
index 65ecdb0cd..335d57230 100644
--- a/test/unit/util/option.js
+++ b/test/unit/util/option.js
@@ -150,8 +150,7 @@ describe('(libs/FP) Option type', () => {
     });
   });
 
-  describe('should.js interactions', () => {
-
+  describe('assertion library interactions', () => {
     // N.B. should.equal() is different from should.eql():
     //
     // * .eql(): check equality using ===
@@ -159,7 +158,11 @@ describe('(libs/FP) Option type', () => {
     //
     // See: https://www.npmjs.com/package/should-equal
 
-    describe('should.eql()', () => {
+    // TODO re-introduce this line when chai is added to the project
+    //const chaiAssert = require('chai').assert;
+    const nodeAssert = require('node:assert');
+
+    describe('equality', () => {
       [
         true,
         false,
@@ -168,9 +171,18 @@ describe('(libs/FP) Option type', () => {
         '',
         'non-empty string',
       ].forEach(val => {
-        it(`should recognise two Options of '${val}' to be equal`, () => {
+        it(`should.js should recognise two Options of '${val}' to be equal`, () => {
           Option.of(val).should.eql(Option.of(val));
         });
+
+        // TODO enable this test when chai is introduced to the project
+        //it(`chai should recognise two Options of '${val}' to be equal`, () => {
+        //  chaiAssert.deepEqual(Option.of(val), Option.of(val));
+        //});
+
+        it(`node:assert should recognise two Options of '${val}' to be equal`, () => {
+          nodeAssert.deepStrictEqual(Option.of(val), Option.of(val));
+        });
       });
 
       [
@@ -179,9 +191,18 @@ describe('(libs/FP) Option type', () => {
         [ 0, '' ],
         [ false, '' ],
       ].forEach((a, b) => {
-        it(`should not recognise Options of '${a}' and '${b}' as equal`, () => {
+        it(`should.js should not recognise Options of '${a}' and '${b}' as equal`, () => {
           Option.of(a).should.not.eql(Option.of(b));
         });
+
+        // TODO enable this test when chai is introduced to the project
+        //it(`chai should not recognise Options of '${a}' and '${b}' as equal`, () => {
+        //  chaiAssert.notDeepEqual(Option.of(a), Option.of(b));
+        //});
+
+        it(`node:assert should not recognise Options of '${a}' and '${b}' as equal`, () => {
+          nodeAssert.notDeepStrictEqual(Option.of(a), Option.of(b));
+        });
       });
     });
   });

From c917170fa39c38921d615ab2aa9c82631802431a Mon Sep 17 00:00:00 2001
From: alxndrsn <alxndrsn>
Date: Wed, 20 Nov 2024 05:45:42 +0000
Subject: [PATCH 3/3] fix equal comment

---
 test/unit/util/option.js | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/unit/util/option.js b/test/unit/util/option.js
index 335d57230..24ce05932 100644
--- a/test/unit/util/option.js
+++ b/test/unit/util/option.js
@@ -153,8 +153,8 @@ describe('(libs/FP) Option type', () => {
   describe('assertion library interactions', () => {
     // N.B. should.equal() is different from should.eql():
     //
-    // * .eql(): check equality using ===
-    // * .eql(): check equality using "should-equal" module
+    // * .equal(): check equality using ===
+    // * .eql():   check equality using "should-equal" module
     //
     // See: https://www.npmjs.com/package/should-equal