Update a test to fail #14843

[hjdivad]

Update a test to fail #14843 and provide example code to illustrate the change suggested in that issue.

Before this can be merged we'd need to decide to deprecate the current semantics of firstObject and lastObject. The trade-off is between having objectAt called in nearly every case for array mutations, or not detecting that [A, B].insertAt(0, A) does not change firstObject.

If that tradeoff is made, we should probably feature flag & deprecate the existing behaviour.

[stefanpenner]

Left some notes, if we decide to proceed.

Would love @krisselden's input

[stefanpenner]

should we still include objectAt(array, 0) !== cacheFor.get(cache, 'firstObject') guard?

[stefanpenner]

should we still include objectAt(array, get(array, 'length') - 1) !== cacheFor.get(cache, 'lastObject')) { guard ?

[stefanpenner]

we should likely avoid this get if we are not needing it.

This is getting complicated, if we go down this path we should do a helper function that can be unit tested (and more readable).

[stefanpenner]

we will require more test permutations, as the above code has many bounds

[hjdivad]

Three possible courses of action:

1. do nothing
2. check the index ranges before calling objectAt
3. remove objectAt entirely and only check index ranges

[homu]

☔ The latest upstream changes (presumably #15210) made this pull request unmergeable. Please resolve the merge conflicts.

[stefanpenner]

@bekzod if you happen to have cycles. I think we would like to move forward with this PR, maybe you can lend @hjdivad a hand and help him land it?

[bekzod]

@stefanpenner sure, will look at it

[bekzod]

maybe following code is a bit clearer (tested with new test) ? @stefanpenner @hjdivad

 if (cache !== undefined) {
    if (cache.firstObject !== undefined && startIdx === 0 && objectAt(array, 0) !== cacheFor.get(cache, 'firstObject')) {
      propertyWillChange(array, 'firstObject');
      propertyDidChange(array, 'firstObject');
    }

    if (cache.lastObject !== undefined) {
      let delta = (addAmt === -1 ? 0 : addAmt) - (removeAmt === -1 ? 0 : removeAmt);
      let length = get(array, 'length');

      let lastIndex = length - delta - 1;
      if ((lastIndex === -1 || delta === 0 || !(startIdx < lastIndex && lastIndex < length)) && objectAt(array, length - 1) !== cacheFor.get(cache, 'lastObject')) {
        propertyWillChange(array, 'lastObject');
        propertyDidChange(array, 'lastObject');
      }
   }
  }

also can startIdx be negative in arrayContentDidChange ?

[bekzod]

I think the approach suggested is good enough, cannot think of better way for now will improve this once merged if I get any better ideas

[stefanpenner]

@bekzod nice, are you able to do the rebase + fix any failing tests? Feel free to open a new PR.

[hjdivad]

Closed in favour of #15510

[hjdivad]

also can startIdx be negative in arrayContentDidChange ?

Yes, the semantics mirror those of splice