Deprecate Ember.Copyable and copy

[locks]

Deprecate Ember.Copyable, Ember.copy, and import { copy } from '@ember/object/internals';

Usage searches

• Ember.Copyable
• Ember Observer (Ember.copy)
• Ember Observer (@ember/object/internals)

[lupestro]

Last night, @runspired noted that ember-data uses Ember.copy for deep copies. It doesn't use Copyable directly but the interfaces for serializers can end up with Ember.Object instances across them, which would need to use Copyable. He also noted that people are likely to use Ember.Object instances in mocks and tests, potentially a source of a lot of rework.

Shallow copies (copy(x, false)) have a direct ES6 replacement in Object.assign, but there is none for deep copies (copy(x, true)). Out on SO, there is actually quite a lengthy discussion about the various options for what deep copies could mean, the handling of functions on objects vs prototypes. The ability to observe members of an EmberObject may make a one-size-fits-all deep copy even less feasible, which I am guessing is why Copyable appeared in the first place.

Anyway, this is all fodder for the RFC I will open this evening. It would appear that Ember only uses Copyable internally for NativeArray. We will need an add-on for those who continue to rely on this feature in their own code. We will need a concrete plan for the relationship between deep copies in Ember Data and the data passed across the interfaces. It would be useful to include some good "cookbook" solutions for common situations.

[lupestro]

I always like to ask the "should we?" question before proceeding to "shall we".

FYI - Deep clone discussion on SO: (https://stackoverflow.com/questions/122102/what-is-the-most-efficient-way-to-deep-clone-an-object-in-javascript) gives some insight into the questions involved.

Looking at the source, Ember.copy(x, true) for non Ember.Object instances seems to handle graphs well, one of the pitfalls of some of the other approaches. I need to take another look at how it handles prototypes, but it would surprise me if that weren't fine, too. In any case, it's a credible response to what is not yet a problem with a clear, general solution.

[krisselden]

Copyable basically is cruft from Sproutcore. It is using mixin as a marker, the only reason array mixes it in is for the copy but isArray() and map() could just be in the copy code which isn't core to Ember can be easily moved to an add-on. Having a copy() method on the array mixin is particularly bad because it makes it impossible to tree shake. Also, Ember Data could make a better model copier.

[runspired]

To clarify what I discussed last night RE copyable and use of Ember.copy for deep cloning within ember-data.

• I am absolutely in favor of this RFC from the perspective of ember-data
• My note is simply that today, users do Bad Things™ in mocks, tests, and in setting some attrs in which we are given essentially classes instead of pojos and arrays. In most cases, users are unaware that they are doing something bad or doing this at all. These users are likely to experience churn and wonder why. This is actually a good thing, their code will be less brittle and buggy. It's not something ember-data ever intentionally supported, but it grew out of our use of copy for deep-cloning that these mistakes go largely uncaught.
• In regards to @krisselden's observation that ember-data could make a better copy more tailored to it's needs, I absolutely agree.

[lupestro]

Thanks for the clarifications, folks. I'll make it a point to study the array code and understand the layers there. As things stand right now, my rationale for proceeding is along the lines of:

"Deep copy is a problem worth solving, but it is a general JavaScript problem. Ember doesn't need to offer a solution, especially one Ember isn't using internally. Ember itself has avoided deep copies rather than using the copy()/Copyable mechanism. The ember-data team is eager to move away from the mechanism because it encourages its users to do things that might work only incidentally, perhaps unaware that they are stepping beyond what's expected to work. copy() and the Copyable mixin are a leftover from the SpoutCore days and there is no compelling reason for them to remain a part of Ember. They can move to an add-on."

So cool! Let's do it. I'll get to work on the RFC itself.

[rwjblue]

Awesome summary, thanks for working on this @lupestro!

[rwjblue]

I believe this is completed, @lupestro can you confirm?

[lupestro]

The RFC is complete.

The implementation checklist is in emberjs/ember.js#16517. I’m waiting for a merge or two, the addon needs a home, but the feature is fully deprecated.

I raised a question about the eslint work, which somebody else could do better than I can, if we want to do it at all. If we change scope slightly, I may need to do an edit or two on the RFC to reflect that.

Otherwise the whole thing, RFC and implementation, is headed to closure.

[rwjblue]

Agree RE: eslint rule. IMHO it is not required. "real world usage" is so low that the deprecation is almost certainly "good enough"...

[RobbieTheWagner]

Where is the addon for ember-copy? I'm getting the deprecations on ember canary, but without the addon, I cannot fix them.

[runspired]

@rwwagner90 unsure where the addon is but the long term solution is to not use copy in this way at all. Deep copy is usually a bad pattern.

That said, I did this for the few parts of ember-data’s test suite where I haven’t had time to refactor yet: https://github.com/emberjs/data/pull/5436/files

[lupestro]

@rwwagner90 The codemod is out on Github at https://github.com/lupestro/ember-copy, but I've asked for @rwjblue to move it to a more central place before publishing it on NPM. It feels to me like repositories for polyfill codemods for deprecated ember.js features should live out in the emberjs space. With angle brackets and so many other awesome things, I suspect Rob's had a busy couple of weeks. 🙂

It's the first codemod I've ever written, and the first GitHub project I've ever posted for general consumption. When published, it will be the first project I've published on npm. Fortunately, it's tiny, as I've been a bit timid about the whole process side of things. Admittedly, I could have tried to coordinate the timing of the code change and the code mod a bit better. Hopefully, we can get this corrected very soon.

That said, I endorse @runspired in his recommendations. 👍 Getting out of copy(true) as soon as possible is a good idea - wherever feasible.

[ro0gr]

seems can be closed, since the RFC(#322) is merged

[locks]

Thanks for spotting it!