Get firstObject/lastObject notification only when cached

[opichals]

Created Em.Array#arrayFirstLastObjectChange abstraction in order to be able to override the Em.Array#firstObject/lastObject notification implementation which by default calls #objectAt(0) and #objectAt(length-1).

These calls are not optimal in case of e.g. sparse RecordArrays which would trigger unnecessary content[index] requests.

Consider an example of using Ember.ListView which only pulls the rendered items out of the content array. In case the content is an ArrayController which dynamically loads items as rendered the Em.Array would actually request the index of 0 and length-1 even in cases where those index content items are not to be rendered at all and therefore trigger unnecessary store layer roundtrips.

[bradleypriest]

Is this related to #5379?

[opichals]

Yes, it is basically the same thing.

I have updated the PR with a test and a variation of the #5379 mentioned diff https://gist.github.com/mmun/877bad33158578a8609b. The cache checks should IMO however do only presence check, not using the cache value (compare with undefined).

[wagenet]

@mmun ping

[petrvolny]

Hey, is there anything that blocks this from merging?

[rwjblue]

Looks like this needs another rebase. I'll to mention this in the upcoming meeting to see if folks are generally 👍 or 👎...

[opichals]

rebased...

[krisselden]

@opichals the cache check is not correct, we should also provide in meta an isCached method, so this mistake isn't copied anymore. The idea should be if we don't think it is safe to recheck, we just fire the change event. It should not be that people can override the firing of the event, it should just be to override rechecking it against objectAt(), we could even limit the recheck to just the NativeArray mixin

[krisselden]

@rwjblue I've reviewed it now, what I mentioned needs to be addressed before merging.

[opichals]

Did a quick cache['x'] !== undefined necessary change. The other comments could be addressed in another PR.

[homu]

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

[opichals]

@krisselden

The idea should be if we don't think it is safe to recheck, we just fire the change event. It should not be that people can override the firing of the event, it should just be to override rechecking it against
objectAt()

Firing the fake change would cause the user of SparseArray to call the get('firstObject') and get('lastObject') which would not prevent the on-deman load of the items as described in the last paragraph of the description of this PR.

rebased

[krisselden]

@opichals I agree it should have the same lazy behavior as a CP and not notify if nothing has actually computed firstObject. I agree with the test.

What I'm against is making this hook public API so that it is ok to prevent a change event even when something has consumed firstObject or lastObject. Checking for undefined is actually a presence check, undefined is cached using a special internal value since in checks are expensive.

If the test and fix were separate from trying to introduce a new public API, I would be happy to merge this. /cc @mmun @rwjblue

[opichals]

@krisselden Thanks, agreed. Updated and rebased.

[rwjblue]

We use 2 space indentation, the 4 spaces here (and below) are causing the build failures).

[rwjblue]

Ping @opichals (I think that this is the main blocker).

[acorncom]

@opichals Tests seem to be failing here, could you fix those?

[krisselden]

It looks like a circular reference issue with the module imports.

[homu]

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

[stefanpenner]

i'll take this

[stefanpenner]

rebase and tracking this over here -> #14493