remove ember-copy dependency #387
Conversation
| @@ -18,7 +20,7 @@ export default class BarBasic extends Component { | |||
| } | |||
| }; | |||
|
|
|||
| chartData = copy(stockData, true); | |||
| chartData = clone(stockData); | |||
There was a problem hiding this comment.
@Gaurav0 I know this is an old PR, but is there any chance there is anything structuredClone doesn't support in this chart data? I know it won't copy functions for example.
There was a problem hiding this comment.
While many things don't work with structuredClone, it is faster and better than ember-copy in what it does support.
There was a problem hiding this comment.
Sure, but is there anything we will break by making the switch?
There was a problem hiding this comment.
Anything that uses copy or the Copyable mixin, without directly adding ember-copy as a dependency, maybe?
There was a problem hiding this comment.
Keep in mind that functions, DOM nodes, etc. cannot really be "cloned" in JavaScript. ember-copy creates a reference in the new object to the original, which "silently works" in some cases, but breaks when you actually switch JavaScript contexts. IMO, an error is better. And chart data shouldn't contain these anyway. However, it is up to you to decide whether this is a "bugfix" or a "breaking change". My 2 cents is document it and call it a bugfix.
There was a problem hiding this comment.
I'm fine with that. Just wanted to double check. Thanks!
Gaurav0
force-pushed
the
remove_ember_copy
branch
from
555aab6 to
e107c70
Compare
|
@RobbieTheWagner rebased. |
Removes unnecessary dependency on ember-copy, simplifying maintainability.