Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 7 additions & 0 deletions packages/@ember/-internals/deprecations/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: '8.0.0',
url: 'https://deprecations.emberjs.com/id/deprecate-comparable-mixin',
}),
};

export function deprecateUntil(message: string, deprecation: DeprecationObject) {
Expand Down
9 changes: 9 additions & 0 deletions packages/@ember/-internals/runtime/lib/mixins/comparable.ts
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this cant be deleted until after a major after a deprecation lands -- like:

https://github.com/emberjs/ember.js/pull/20702/changes#diff-aa3f9176575f028f979266539b2a7647d98ec70c72162bd89de74f863c857f8b

and then at the major we can remove like here:

#20891

Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import Mixin from '@ember/object/mixin';
import { deprecateUntil, DEPRECATIONS } from '@ember/-internals/deprecations';

/**
@module ember
Expand Down Expand Up @@ -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,
});

Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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);
Expand Down
25 changes: 22 additions & 3 deletions packages/@ember/utils/lib/compare.ts
Original file line number Diff line number Diff line change
@@ -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> = {
Expand Down Expand Up @@ -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'
);
}
10 changes: 8 additions & 2 deletions packages/@ember/utils/tests/compare_test.js
Original file line number Diff line number Diff line change
@@ -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);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this part could be extracted to a separate pr tho ❤️

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I’d prefer to keep this here because after adding the deprecation this test fails if it keeps using Comparable - Comp.create() triggers the deprecation.

let Comp = EmberObject.extend();

Comp.reopenClass({
compare(obj) {
Expand Down Expand Up @@ -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');
}
}
);
Loading