Prefer generic insertAt over prototype_extensions#16048
Prefer generic insertAt over prototype_extensions#16048wongpeiyi wants to merge 4 commits intoemberjs:masterfrom
insertAt over prototype_extensions#16048Conversation
|
@Serabe can you see if this is in line with what the epic requires? Don't think Would like to take a stab at the remainder in this fashion if this looks ok |
| suite.module('insertAt'); | ||
|
|
||
| suite.test('[].insertAt(0, X) => [X] + notify', function() { | ||
| suite.test('insertAt([], 0, X) => [X] + notify', function() { |
There was a problem hiding this comment.
I think its beneficial to have some tests still use the array prototype until it is fully removed. I'm not sure if we have tests that specific check for this already elsewhere or if we should add those checks here
There was a problem hiding this comment.
I followed the example of https://github.com/emberjs/ember.js/pull/13534/files#diff-d1dc85945ef368d3bcdd3e01234d3f8eR118 and duplicated the last test, keeping it to use the array prototype extension. See https://github.com/emberjs/ember.js/pull/16048/files/ac0a1455b73d59a5b1256f81096343a90f846ebd#diff-eaa813784954ae3195242cb1d1763b7dR171
I agree it's not very clear though. I can change the test's label but not sure if the test itself needs to be more specific.
There was a problem hiding this comment.
I guess what I'm trying to get at is that we should maintain at least one test that simply checks that the prototypes are still extended. This could simply be something like:
test('that prototypes are properly extended', function(assert) {
assert.equal(typeof [].insertAt, 'function');
// ... all other extensions
});I don't know if we have something like this already but if not we should add it
There was a problem hiding this comment.
Do you mean something like https://github.com/emberjs/ember.js/blob/master/packages/ember-runtime/tests/system/string/camelize_test.js#L6-L10 but the inverse?
Added a commit for that for both removeAt and insertAt
|
LGTM. Need approval from core yet. |
|
I see with my little eye some git conflicts. Can you take a look @wongpeiyi? |
wongpeiyi
force-pushed
the
array-internals-insertAt
branch
3 times, most recently
from
80207d6 to
d12f046
Compare
|
@locks thanks, rebased. |
| return array; | ||
| } | ||
|
|
||
| return array.splice(idx, amt, ...objects); |
There was a problem hiding this comment.
Hmm, when the array has .replace we return the mutated array, but if it doesn't we return the deleted elements.
Can you tweak this so that its consistent?
There was a problem hiding this comment.
Thanks, added a commit to address that. Also brought it more in line with NativeArray
@rwjblue tagging in case this was missed. Had to rebase again, tests failing because of master
| import Enumerable from './enumerable'; | ||
| import { Error as EmberError } from 'ember-debug'; | ||
|
|
||
| function replace(array, idx, amt, objects) { |
There was a problem hiding this comment.
I would have expected this to be much closer to NativeArray#replace...
wongpeiyi
force-pushed
the
array-internals-insertAt
branch
2 times, most recently
from
ef3e851 to
9d9c96f
Compare
wongpeiyi
force-pushed
the
array-internals-insertAt
branch
from
9d9c96f to
0f23feb
Compare
|
@rwjblue can I get your eyes on this again? Had to rebase, and tests failed because of master at that time |
|
@wongpeiyi FYI, I think we're going to take a slightly different approach. I will lay the foundation down soon and possibly start up a new quest issue. |
|
Sorry for the run around here @wongpeiyi |
6 participants
#15501
insertAtDuplicated one test for testing the prototype extension still works.