[BUGFIX] Fix EmberArray.reduce to match native behavior - #21168
Conversation
|
@NullVoxPopuli Please Have a Review On this PR and Let me Know if this requires any Update or changes |
| - uses: actions/checkout@v6 | ||
| - uses: ./.github/actions/setup | ||
| - uses: wyvox/pkg-size@029acee084efc5ab835fee5f515ae5c3fc7549c1 | ||
| continue-on-error: true |
There was a problem hiding this comment.
Please undo this change -- i have a fix here: #21166
There was a problem hiding this comment.
Done! I've reverted the package-size.yml changes in my latest commit.
|
For my own context, any reason to use ember array instead of trackedArray? Reason i ask is that we're aiming to eliminate EmberArray during v7, hopefully for removal in v8 i think. If you need the reactivity, trackedArray should hopefully solve this as well. What motivated this change to EmberArray? |
| } | ||
|
|
||
| for (let i = startIndex; i < this.length; i++) { | ||
| let item = this.objectAt(i) as T; |
There was a problem hiding this comment.
Why objectAt instead of [] access? Oc .at()
There was a problem hiding this comment.
Since EmberArray is a mixin (used by ArrayProxy, etc.), it doesn't always guarantee that native [] bracket access or .at() will work to retrieve elements. objectAt is the safest and standard method defined by the Enumerable/EmberArray API for retrieving items.
| this.assert.equal(res, obj); | ||
| } | ||
|
|
||
| '@test works without an initialValue'() { |
There was a problem hiding this comment.
| let obj = this.newObject([1, 2, 3]); | ||
| let res = obj.reduce((previousValue, item, index) => previousValue + index); | ||
| // starts at item 2 (index 1): 1 + 1 = 2 | ||
| // then item 3 (index 2): 2 + 2 = 4 |
There was a problem hiding this comment.
TIL reduce without initialValue starts iteration at 1st index rather than 0 (because 0th index is used for initial value)
There was a problem hiding this comment.
Tests match MDN's destribed behavior ✅
|
I noticed there was a long-standing FIXME comment right above the EmberArray#reduce method (packages/@ember/array/index.ts:1386) stating that the behavior didn't match native Array.prototype.reduce when called without an initialValue. I'm just exploring the codebase and trying to clean up some of these old FIXME/TODO comments! |
|
@crazylogic03 can you rebase this PR? the pkg-size update was merged so your change to that workflow isn't needed, and I fear would cause a conflict -- (or GH isn't diffing correctly, idk) |
1ba1bb7 to
b58a197
Compare
|
@NullVoxPopuli Done! I've removed the pkg-size update and rebased the commits against the latest main. Everything should be clean now! |
kategengler
changed the title
2 participants
Description
Fixes a FIXME where
EmberArray#reducedid not match the nativeArray.prototype.reducebehavior when noinitialValuewas provided. Seepackages/@ember/array/index.ts.initialValueis provided and the array is empty, it now throws aTypeError(consistent with native).initialValueis provided, it uses the first item as the initial value and begins iterating from index 1.Tested with new test cases in
reduce-test.js.Verification
pnpm lint:format:fixandpnpm testwere run to ensure formatting is correct and no regressions were introduced.