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

replace all uses of Ember.copy, Ember.merge w/ Object.assign #835

Merged
merged 1 commit into from
Jul 25, 2018

Conversation

bgentry
Copy link
Contributor

@bgentry bgentry commented Jul 24, 2018

Attempted fix of #834. Not sure if I have to do something weird for the import in ember_debug/libs/promise-assembler.js or not…

@bgentry
Copy link
Contributor Author

bgentry commented Jul 24, 2018

FYI, these same test failures appear to also happen on master. I'll open a separate issue for that.

@@ -6,8 +6,9 @@
*/

import Promise from 'ember-debug/models/promise';
import { copy } from 'ember-copy';
Copy link
Member

Choose a reason for hiding this comment

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

@teddyzeenny should this import work here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it would, no. Not unless the app being debugged has ember-copy installed as an addon.

Copy link
Member

Choose a reason for hiding this comment

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

So what is your recommendation here?

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no way to use Ember addons in ember-debug at the moment. We may be able to eventually work this out by custom-building and including the addons in the ember-debug final build, but I doubt it's going to be simple.

For the sake of this PR, my recommendation would just be to not use Ember.copy in this file as it's not even doing a deep clone. So either properties = {...properties} if our Babel setup supports it, or a simple manual shallow copy by iterating over the properties. Either way, I don't think this function requires an Ember addon.

@bgentry
Copy link
Contributor Author

bgentry commented Jul 25, 2018 via email

@teddyzeenny
Copy link
Contributor

@bgentry yes Object.assign works! 👍

@RobbieTheWagner
Copy link
Member

@teddyzeenny with that in mind, does that work everywhere? Could we completely remove ember-copy in favor of that?

@teddyzeenny
Copy link
Contributor

ember-copy can do deep clones which Object.assign and { ...spread } cannot, I think that's the difference? If they're all shallow copies we should be able remove ember-copy.

@bgentry bgentry changed the title install ember-copy, replace all uses of Ember.copy replace all uses of Ember.copy, Ember.merge w/ Object.assign Jul 25, 2018
@bgentry
Copy link
Contributor Author

bgentry commented Jul 25, 2018

I updated this PR. It no longer uses ember-copy, instead just using Object.assign everywhere that copy was previously. I'm not certain whether this is ok (if everything is just a shallow copy) so somebody w/ more knowledge should verify that 😄

@RobbieTheWagner
Copy link
Member

@bgentry sorry to keep making you change things, but I think we should keep ember-copy in the app code, but just use Object.assign in the ember_debug code, just to be safe. With those changes, I would be fine with merging, since @teddyzeenny said Object.assign was okay for that one case.

@RobbieTheWagner
Copy link
Member

As I was typing that, Teddy approved the PR 😂. Let me sync with him offline.

@RobbieTheWagner
Copy link
Member

@teddyzeenny thinks this is good to go, and we don't need deep copies, so works for me 👍

@RobbieTheWagner RobbieTheWagner merged commit c393f0e into emberjs:master Jul 25, 2018
@bgentry bgentry deleted the ember-copy-deprecation branch July 25, 2018 18:18
cyril-sf pushed a commit to cyril-sf/ember-inspector that referenced this pull request Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants