From 641efea3e89ab6dcc2b26ae186cf56769ea6d67c Mon Sep 17 00:00:00 2001 From: Ralph Mack Date: Sat, 26 May 2018 15:25:46 -0400 Subject: [PATCH 1/2] Implemented copy/Copyable deprecation --- packages/ember-routing/lib/system/route.js | 3 +-- packages/ember-runtime/lib/copy.js | 24 ++++++++++++------- packages/ember-runtime/lib/mixins/array.js | 1 - packages/ember-runtime/lib/mixins/copyable.js | 1 + .../ember-runtime/tests/core/copy_test.js | 24 ++++++++++++++----- .../tests/routing/decoupled_basic_test.js | 16 ++++++------- 6 files changed, 44 insertions(+), 25 deletions(-) diff --git a/packages/ember-routing/lib/system/route.js b/packages/ember-routing/lib/system/route.js index b7b850be6b2..bc7fffa0a11 100644 --- a/packages/ember-routing/lib/system/route.js +++ b/packages/ember-routing/lib/system/route.js @@ -8,7 +8,6 @@ import { DEBUG } from '@glimmer/env'; import { classify } from '@ember/string'; import { typeOf, - copy, Object as EmberObject, A as emberA, Evented, @@ -1593,7 +1592,7 @@ let Route = EmberObject.extend(ActionHandler, Evented, { if (!name) { if (sawParams) { - return copy(params); + return Object.assign({}, params); } else { if (transition.resolveIndex < 1) { return; diff --git a/packages/ember-runtime/lib/copy.js b/packages/ember-runtime/lib/copy.js index cd6cb378777..dde0a2547c2 100644 --- a/packages/ember-runtime/lib/copy.js +++ b/packages/ember-runtime/lib/copy.js @@ -1,10 +1,12 @@ -import { assert } from '@ember/debug'; +import { assert, deprecate } from '@ember/debug'; import EmberObject from './system/object'; import Copyable from './mixins/copyable'; /** @module @ember/object -*/ + @private + @deprecated Use 'ember-copy' addon instead + */ function _copy(obj, deep, seen, copies) { // primitive data types are immutable, just return them. if (typeof obj !== 'object' || obj === null) { @@ -18,11 +20,6 @@ function _copy(obj, deep, seen, copies) { return copies[loc]; } - assert( - 'Cannot clone an EmberObject that does not implement Copyable', - !(obj instanceof EmberObject) || Copyable.detect(obj) - ); - // IMPORTANT: this specific test will detect a native array only. Any other // object will need to implement Copyable. if (Array.isArray(obj)) { @@ -40,6 +37,11 @@ function _copy(obj, deep, seen, copies) { } else if (obj instanceof Date) { ret = new Date(obj.getTime()); } else { + assert( + 'Cannot clone an EmberObject that does not implement Copyable', + !(obj instanceof EmberObject) || Copyable.detect(obj) + ); + ret = {}; let key; for (key in obj) { @@ -86,12 +88,18 @@ function _copy(obj, deep, seen, copies) { @public */ export default function copy(obj, deep) { + deprecate('Use ember-copy addon instead of copy method and Copyable mixin.', false, { + id: 'ember-runtime.deprecate-copy-copyable', + until: '4.0.0', + url: 'https://emberjs.com/deprecations/v3.x/#toc_ember-runtime-deprecate-copy-copyable', + }); + // fast paths if ('object' !== typeof obj || obj === null) { return obj; // can't copy primitives } - if (Copyable.detect(obj)) { + if (!Array.isArray(obj) && Copyable.detect(obj)) { return obj.copy(deep); } diff --git a/packages/ember-runtime/lib/mixins/array.js b/packages/ember-runtime/lib/mixins/array.js index f2ba6ec6c6f..5583e86ad19 100644 --- a/packages/ember-runtime/lib/mixins/array.js +++ b/packages/ember-runtime/lib/mixins/array.js @@ -1628,7 +1628,6 @@ const MutableArray = Mixin.create(ArrayMixin, MutableEnumerable, { @class Ember.NativeArray @uses MutableArray @uses Observable - @uses Ember.Copyable @public */ let NativeArray = Mixin.create(MutableArray, Observable, { diff --git a/packages/ember-runtime/lib/mixins/copyable.js b/packages/ember-runtime/lib/mixins/copyable.js index 080ad5ab3ec..dd6583db1f2 100644 --- a/packages/ember-runtime/lib/mixins/copyable.js +++ b/packages/ember-runtime/lib/mixins/copyable.js @@ -15,6 +15,7 @@ import { Mixin } from 'ember-metal'; @class Copyable @namespace Ember @since Ember 0.9 + @deprecated Use 'ember-copy' addon instead @private */ export default Mixin.create({ diff --git a/packages/ember-runtime/tests/core/copy_test.js b/packages/ember-runtime/tests/core/copy_test.js index 7e665b8994a..6c7a71536fe 100644 --- a/packages/ember-runtime/tests/core/copy_test.js +++ b/packages/ember-runtime/tests/core/copy_test.js @@ -6,14 +6,19 @@ moduleFor( class extends AbstractTestCase { ['@test Ember.copy null'](assert) { let obj = { field: null }; - - assert.equal(copy(obj, true).field, null, 'null should still be null'); + let copied = null; + expectDeprecation(() => { + copied = copy(obj, true); + }, 'Use ember-copy addon instead of copy method and Copyable mixin.'); + assert.equal(copied.field, null, 'null should still be null'); } ['@test Ember.copy date'](assert) { let date = new Date(2014, 7, 22); - let dateCopy = copy(date); - + let dateCopy = null; + expectDeprecation(() => { + dateCopy = copy(date); + }, 'Use ember-copy addon instead of copy method and Copyable mixin.'); assert.equal(date.getTime(), dateCopy.getTime(), 'dates should be equivalent'); } @@ -21,13 +26,20 @@ moduleFor( let obj = Object.create(null); obj.foo = 'bar'; + let copied = null; + expectDeprecation(() => { + copied = copy(obj); + }, 'Use ember-copy addon instead of copy method and Copyable mixin.'); - assert.equal(copy(obj).foo, 'bar', 'bar should still be bar'); + assert.equal(copied.foo, 'bar', 'bar should still be bar'); } ['@test Ember.copy Array'](assert) { let array = [1, null, new Date(2015, 9, 9), 'four']; - let arrayCopy = copy(array); + let arrayCopy = null; + expectDeprecation(() => { + arrayCopy = copy(array); + }, 'Use ember-copy addon instead of copy method and Copyable mixin.'); assert.deepEqual(array, arrayCopy, 'array content cloned successfully in new array'); } diff --git a/packages/ember/tests/routing/decoupled_basic_test.js b/packages/ember/tests/routing/decoupled_basic_test.js index 091001ea2da..a6bea36c6f1 100644 --- a/packages/ember/tests/routing/decoupled_basic_test.js +++ b/packages/ember/tests/routing/decoupled_basic_test.js @@ -5,7 +5,7 @@ import { compile } from 'ember-template-compiler'; import { ENV } from 'ember-environment'; import { Route, NoneLocation, HistoryLocation } from 'ember-routing'; import Controller from '@ember/controller'; -import { Object as EmberObject, A as emberA, copy } from 'ember-runtime'; +import { Object as EmberObject, A as emberA } from 'ember-runtime'; import { moduleFor, ApplicationTestCase, runDestroy } from 'internal-test-helpers'; import { run } from '@ember/runloop'; import { Mixin, computed, set, addObserver, observer } from 'ember-metal'; @@ -1035,8 +1035,7 @@ moduleFor( actions: { showStuff(obj) { assert.ok(this instanceof HomeRoute, 'the handler is an App.HomeRoute'); - // Using Ember.copy removes any private Ember vars which older IE would be confused by - assert.deepEqual(copy(obj, true), { name: 'Tom Dale' }, 'the context is correct'); + assert.deepEqual(Object.assign({}, obj), { name: 'Tom Dale' }, 'the context is correct'); done(); }, }, @@ -1070,8 +1069,7 @@ moduleFor( actions: { showStuff(obj) { assert.ok(this instanceof RootRoute, 'the handler is an App.HomeRoute'); - // Using Ember.copy removes any private Ember vars which older IE would be confused by - assert.deepEqual(copy(obj, true), { name: 'Tom Dale' }, 'the context is correct'); + assert.deepEqual(Object.assign({}, obj), { name: 'Tom Dale' }, 'the context is correct'); done(); }, }, @@ -1210,10 +1208,12 @@ moduleFor( actions: { showStuff(obj1, obj2) { assert.ok(this instanceof RootRoute, 'the handler is an App.HomeRoute'); - // Using Ember.copy removes any private Ember vars which older IE would be confused by - assert.deepEqual(copy(obj1, true), { name: 'Tilde' }, 'the first context is correct'); assert.deepEqual( - copy(obj2, true), + Object.assign({}, obj1), + { name: 'Tilde' }, + 'the first context is correct'); + assert.deepEqual( + Object.assign({}, obj2), { name: 'Tom Dale' }, 'the second context is correct' ); From f6f60a7d23cc11c1e54bca8f7109478d557d742e Mon Sep 17 00:00:00 2001 From: Ralph Mack Date: Sat, 26 May 2018 16:18:10 -0400 Subject: [PATCH 2/2] Cleaned up a couple of "pretty" issues the Travis build found. --- packages/ember-routing/lib/system/route.js | 8 +------- .../tests/routing/decoupled_basic_test.js | 19 ++++++++++++++----- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/packages/ember-routing/lib/system/route.js b/packages/ember-routing/lib/system/route.js index bc7fffa0a11..2bf6b86aef2 100644 --- a/packages/ember-routing/lib/system/route.js +++ b/packages/ember-routing/lib/system/route.js @@ -6,13 +6,7 @@ import { get, set, getProperties, setProperties, computed, isEmpty } from 'ember import { assert, deprecate, info, isTesting } from '@ember/debug'; import { DEBUG } from '@glimmer/env'; import { classify } from '@ember/string'; -import { - typeOf, - Object as EmberObject, - A as emberA, - Evented, - ActionHandler, -} from 'ember-runtime'; +import { typeOf, Object as EmberObject, A as emberA, Evented, ActionHandler } from 'ember-runtime'; import generateController from './generate_controller'; import { stashParamNames, diff --git a/packages/ember/tests/routing/decoupled_basic_test.js b/packages/ember/tests/routing/decoupled_basic_test.js index a6bea36c6f1..b3a5e118156 100644 --- a/packages/ember/tests/routing/decoupled_basic_test.js +++ b/packages/ember/tests/routing/decoupled_basic_test.js @@ -1035,7 +1035,11 @@ moduleFor( actions: { showStuff(obj) { assert.ok(this instanceof HomeRoute, 'the handler is an App.HomeRoute'); - assert.deepEqual(Object.assign({}, obj), { name: 'Tom Dale' }, 'the context is correct'); + assert.deepEqual( + Object.assign({}, obj), + { name: 'Tom Dale' }, + 'the context is correct' + ); done(); }, }, @@ -1069,7 +1073,11 @@ moduleFor( actions: { showStuff(obj) { assert.ok(this instanceof RootRoute, 'the handler is an App.HomeRoute'); - assert.deepEqual(Object.assign({}, obj), { name: 'Tom Dale' }, 'the context is correct'); + assert.deepEqual( + Object.assign({}, obj), + { name: 'Tom Dale' }, + 'the context is correct' + ); done(); }, }, @@ -1209,9 +1217,10 @@ moduleFor( showStuff(obj1, obj2) { assert.ok(this instanceof RootRoute, 'the handler is an App.HomeRoute'); assert.deepEqual( - Object.assign({}, obj1), - { name: 'Tilde' }, - 'the first context is correct'); + Object.assign({}, obj1), + { name: 'Tilde' }, + 'the first context is correct' + ); assert.deepEqual( Object.assign({}, obj2), { name: 'Tom Dale' },