[BUGFIX beta] Don't use prototype extensions (arrayContent{Did,Will}Change, removeAt) #13534
Conversation
| colors.removeAt(0); // ['green', 'blue', 'yellow', 'orange'] | ||
| colors.removeAt(2, 2); // ['green', 'blue'] | ||
| colors.removeAt(4, 2); // Error: Index out of range | ||
| removeAt(colors, 0); // ['green', 'blue', 'yellow', 'orange'] |
There was a problem hiding this comment.
These examples are showing how to use the mutable array mixin itself, I would think that we want to leave them as colors.removeAt(2, 2).
|
I'm not 100% sure on this. It seems like the right direction, but I wouldn't expect so many changes in tests that were already using things extended by |
|
This is much like moving away from global API internally. In general, the tests migrated to the modules API but we should have tests that test we export the API onto |
|
nice! |
|
we should probably get this in ASAP as it is likely to bit quickly with the churn in glimmer 2 PRs |
|
@rwjblue I think NativeArray could have a test but that any other tests not testing NativeArray directly should be using the standalone methods. |
|
I've added one test and updated those comments. |
|
☔ The latest upstream changes (presumably #13542) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Rebased |
| @@ -33,6 +35,40 @@ var EMPTY = []; | |||
|
|
|||
| function K() { return this; } | |||
|
|
|||
| export function removeAt(array, start, len) { | |||
| if ('number' === typeof start) { | |||
There was a problem hiding this comment.
why extract this? it doesn't let you fire the events for this operation without using Ember.A().
There was a problem hiding this comment.
the first argument is still required to be an ember array.
This isn't the equivalent of Ember.set(obj, key, val) where the obj can be a pojo.
There was a problem hiding this comment.
@krisselden to be able to call removeAt(array, 1, 2); in some other files such as arranged_content_test.js, although there might not be any advantage over array.removeAt(1, 2);.
There was a problem hiding this comment.
it if requires it to be an array, there isn't a point to the standalone, there is only value migrating to extend prototypes being false by default if Ember isn't doing Ember.A() all over the place, which is very slow (when extend prototypes is false).
There was a problem hiding this comment.
That makes sense. I've reverted that chage.
|
☔ The latest upstream changes (presumably #13680) made this pull request unmergeable. Please resolve the merge conflicts. |
Add test for `.removeAt` using prototype extensions. Update comments.
|
Rebased. |
|
Thanks @btecu, sorry for letting this linger so long. |
|
I'm not sure where or how to direct this feedback, so I'm going to do so here. If there is a better place for this discussion please let me know. These commits are problematic for ember-data-model-fragments. The root of the problem is that because Ultimately I acknowledge that this an model-fragments issue. We were overriding a public API and now the way it's invoked internally from ember has change. So I don't think this is exactly API breakage, but I can definitely see this as problematic for anyone else in a similar situation. |
|
@workmanw that sounds like a breaking change to me |
|
@stefanpenner If that's the case, I'd like to get this fixed up before ember 2.8 ships so it doesn't become an upgrade blocker for us, or anyone else. I can submit a PR to revert part of this commit so that internally ember will call |
|
Seems unlikely that breaking the overriding of an api can be considered a breaking change. |
|
@stefanpenner it seems so. Though a public API, it is a method to call to notify about the change, not to be notified about it. Interesting issue! |
|
@stefanpenner Yea, absolutely. |
Do not rely on prototype extensions for
removeAt.Related to #9269 and #10899.