Merge pull request #21437 from olenderhub/remove-comparable-mixin

Deprecate the private Comparable mixin and remove internal usage

Author: kategengler

packages/@ember/-internals/deprecations/index.ts

@@ -112,6 +112,13 @@ export const DEPRECATIONS = {
     until: '7.0.0',
     url: 'https://deprecations.emberjs.com/id/importing-inject-from-ember-service',
   }),
+  DEPRECATE_COMPARABLE_MIXIN: deprecation({
+    for: 'ember-source',
+    id: 'deprecate-comparable-mixin',
+    since: { available: '7.2.0', enabled: '7.2.0' },
+    until: '7.5.0',
+    url: 'https://deprecations.emberjs.com/id/deprecate-comparable-mixin',
+  }),
 };
 
 export function deprecateUntil(message: string, deprecation: DeprecationObject) {

packages/@ember/-internals/runtime/lib/mixins/comparable.ts

@@ -1,4 +1,5 @@
 import Mixin from '@ember/object/mixin';
+import { deprecateUntil, DEPRECATIONS } from '@ember/-internals/deprecations';
 
 /**
 @module ember
@@ -37,6 +38,14 @@ const Comparable = Mixin.create({
     @return {Number} the result of the comparison
     @private
   */
+  init() {
+    this._super(...arguments);
+    deprecateUntil(
+      'The `Comparable` mixin is deprecated. Implement a `compare` method directly on your class instead.',
+      DEPRECATIONS.DEPRECATE_COMPARABLE_MIXIN
+    );
+  },
+
   compare: null,
 });
 

packages/@ember/-internals/runtime/tests/mixins/comparable_test.js

@@ -1,7 +1,8 @@
 import EmberObject, { get } from '@ember/object';
 import { compare } from '@ember/utils';
 import Comparable from '../../lib/mixins/comparable';
-import { moduleFor, AbstractTestCase } from 'internal-test-helpers';
+import { moduleFor, AbstractTestCase, expectDeprecation, testUnless } from 'internal-test-helpers';
+import { DEPRECATIONS } from '../../../deprecations';
 
 class Rectangle extends EmberObject.extend(Comparable) {
   length = 0;
@@ -22,11 +23,17 @@ moduleFor(
   'Comparable',
   class extends AbstractTestCase {
     beforeEach() {
+      expectDeprecation(
+        /The `Comparable` mixin is deprecated/,
+        DEPRECATIONS.DEPRECATE_COMPARABLE_MIXIN.isEnabled
+      );
       r1 = Rectangle.create({ length: 6, width: 12 });
       r2 = Rectangle.create({ length: 6, width: 13 });
     }
 
-    ['@test should be comparable and return the correct result'](assert) {
+    [`${testUnless(
+      DEPRECATIONS.DEPRECATE_COMPARABLE_MIXIN.isRemoved
+    )} @test should be comparable and return the correct result`](assert) {
       assert.equal(Comparable.detect(r1), true);
       assert.equal(compare(r1, r1), 0);
       assert.equal(compare(r1, r2), -1);

packages/@ember/utils/lib/compare.ts

@@ -1,6 +1,5 @@
 import type { TypeName } from './type-of';
 import typeOf from './type-of';
-import Comparable from '@ember/-internals/runtime/lib/mixins/comparable';
 import { assert } from '@ember/debug';
 
 const TYPE_ORDER: Record<TypeName, number> = {
@@ -163,10 +162,30 @@ export default function compare<T>(v: T, w: T): Compare {
   }
 }
 
+interface Comparable {
+  compare: (a: unknown, b: unknown) => -1 | 0 | 1;
+}
+
 interface ComparableConstructor {
-  constructor: Comparable;
+  constructor: {
+    compare: (a: unknown, b: unknown) => -1 | 0 | 1;
+  };
 }
 
 function isComparable(value: unknown): value is Comparable & ComparableConstructor {
-  return Comparable.detect(value);
+  if (typeof value !== 'object' || value === null) {
+    return false;
+  }
+
+  let maybeComparable = value as {
+    compare?: unknown;
+    constructor?: {
+      compare?: unknown;
+    };
+  };
+
+  return (
+    typeof maybeComparable.constructor?.compare === 'function' ||
+    typeof maybeComparable.compare === 'function'
+  );
 }

packages/@ember/utils/tests/compare_test.js

@@ -1,10 +1,9 @@
 import { compare, typeOf } from '@ember/utils';
 import EmberObject from '@ember/object';
-import { Comparable } from '@ember/-internals/runtime';
 import { moduleFor, AbstractTestCase } from 'internal-test-helpers';
 
 let data = [];
-let Comp = EmberObject.extend(Comparable);
+let Comp = EmberObject.extend();
 
 Comp.reopenClass({
   compare(obj) {
@@ -84,5 +83,12 @@ moduleFor(
       assert.equal(compare('b', zero), 0, 'Second item comparable - returns  0 (negated)');
       assert.equal(compare('c', one), -1, 'Second item comparable - returns  1 (negated)');
     }
+
+    ['@test non-function compare does not make an object comparable'](assert) {
+      let obj = EmberObject.create({ compare: null });
+      let other = EmberObject.create({ compare: 'not a function' });
+
+      assert.equal(compare(obj, other), 0, 'objects with non-function compare are not comparable');
+    }
   }
 );