Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecation of copy and Copyable #322

Merged
merged 3 commits into from
Apr 27, 2018

Conversation

lupestro
Copy link
Contributor

Here's the initial complete draft

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Wonderfully researched and written @lupestro! Thank you for putting this together.


Those using the add-on would need to mechanically adjust any uses of `myArray.copy(deep)` to `copy(myArray, deep)` in order to avoid the deprecation message.

I cannot see a way to notify the user through deprecation at runtime about their own objects that are extending `Copyable` and contain `copy()` methods that will no longer be called when the deprecated `Ember.copy()` method goes away. Our best bet for this might be to supply a new eslint warning that flags the import of `Copyable`.
Copy link
Member

Choose a reason for hiding this comment

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

We can implement a deprecation at “extend” time such that anything including the copyable mixin received the deprecation (even if copy is never called on it), happy to deep dive the details with you in slack...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be welcome - but that would include all arrays, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"at extend time" - Since extend is defined in system.core-object, also in ember-runtime, implementing a deprecation at that level might be architecturally sound. The check would be a small tax on every extend, though, which seems severe. @rwjblue Was that where you intended to put it? Or somewhere else in the chain of activity "at extend time"?

@rwjblue
Copy link
Member

rwjblue commented Mar 25, 2018

Let’s do this!!

@rwjblue rwjblue self-assigned this Mar 25, 2018
@cibernox
Copy link
Contributor

This looks great. This is a general JS problem that should not be part of Ember.

@sdhull
Copy link

sdhull commented Apr 6, 2018

My initial reaction was "What?! No, I neeeed that!" but yeah I'd be fine switching to an addon. Plus I'd love to change the mechanics of how it copies (or rather that it doesn't copy) arrays. Maybe I'd be better of building my own copy() utility 🤔

@rwjblue
Copy link
Member

rwjblue commented Apr 6, 2018

Discussed with the core team (last Friday) and we are 👍 on moving this into Final Comment Period.

@runspired
Copy link
Contributor

I have removed all usage of copy from ember-data emberjs/data#5436

@rwjblue rwjblue merged commit cbc1973 into emberjs:master Apr 27, 2018
@rwjblue
Copy link
Member

rwjblue commented Apr 27, 2018

Thank for all the hard work here, I super excited to be moving forward!

wagenet added a commit to wagenet/liquid-fire that referenced this pull request Jun 15, 2018
wagenet added a commit to wagenet/ember-svg-jar that referenced this pull request Jun 15, 2018
wagenet added a commit to wagenet/ember-svg-jar that referenced this pull request Jun 15, 2018
wagenet added a commit to wagenet/liquid-fire that referenced this pull request Jun 21, 2018
wagenet added a commit to wagenet/liquid-fire that referenced this pull request Jun 21, 2018
wagenet added a commit to wagenet/liquid-fire that referenced this pull request Jul 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants