Update ember-in-viewport, ember-wormhole, move ember-composable-helpers to devDependencies

[fran-worley]

Suggesting this PR in favour of:
#674
#682

@alexander-alvarez I can't see a way around the api change here with the ember-in-viewport event unless @snewcomer can think of one. I'm not sure if it's specifically an issue with changes to ember-in-viewport or a combination of that and newer emberjs but without this change we were getting Uncaught TypeError: inViewport.startIntersectionObserver is not a function as I assume the mixin was trying to call startIntersectionObserver on our action/ function rather than the imported service.

@snewcomer any ideas why our onScrollToBottom test is now only being hit 3 times. I'm not entirely sure the justification behind it being hit 4 times in the first place...

[snewcomer]

Looks good! On that test, the onScrollToBottom doesn't look like it is being hit. Perhaps instead of the scrollTo helper, you could try something like this. scrollIntoView

https://github.com/ember-infinity/ember-infinity/blob/master/tests/acceptance/infinity-route-test.js#L28

[fran-worley]

@snewcomer that unfortunately doesn't seem to have improved things. I've updated the scrolling tests here - https://github.com/offirgolan/ember-light-table/tree/scrolling-tests not to use ember-native-dom-helpers and the tests in question pass prior to updating ember inViewport but not afterwards... 😢

[fran-worley]

@snewcomer the failings are definitely related to a change in ember-in-viewport between 3.0 and 3.1 I've run some tests locally and I can get everything passing (even with the breaking change to inViewport event name) at v3.0.3 but the onScrollToBottom failures reappear as soon as ember-in-viewport is updated any ideas what changed that could be causing the issue?

Oddly the dummy app seems to have the opposite problem and after upgrading ember-in-viewport onScrollToBottom is called continuously without actually doing any scrolling at all:

[snewcomer]

@fran-worley The infinite loop seems to be b/c of the observer on scheduleScrolledToBottom. We need to remove rows.[]. Second, there are three things I identified to change -

1. Upgrade to 3.5.8 ember-in-viewport
2. change lt-infinity.js

import Component from '@ember/component';
import layout from '../templates/components/lt-infinity';
import { get } from '@ember/object';
import { inject as service } from '@ember/service';

export default Component.extend({
  layout,

  inViewport: service(),

  tagName: '',

  rows: null,
  scrollableContent: null,

  didInsertElement() {
    this._super(...arguments);

    let scrollBuffer = this.get('scrollBuffer');
    let scrollableContent = this.get('scrollableContent');

    const options = {
      viewportSpy: true,
      viewportTolerance: {
        bottom: scrollBuffer
      },
      scrollableArea: scrollableContent
    };

    const { onEnter, onExit } = this.inViewport.watchElement(document.querySelector('.lt-infinity'), options);
    onEnter(this.didEnterViewport.bind(this));
    onExit(this.didExitViewport.bind(this));
  },

  didEnterViewport() {
    get(this, 'enterViewport')();
  },

  didExitViewport() {
    get(this, 'exitViewport')();
  }
});

3. add to lt-infinity.hbs

<div class="lt-infinity"></div>

Lastly there is a bug b/c the "sentinel" (lt-infinity) gets new properties and breaks the cache. So the above won't work until I fix an underlying library and bump in-viewport to 3.5.8

[snewcomer]

@fran-worley If you update ember-in-viewport to 3.5.8 as well as the above changes, this should work. Just tested! 👍

[fran-worley]

You are a genius! I'll take a look at this in the morning and see if we can get a passing build 👯

[fran-worley]

@snewcomer So I've revised the code as per your suggestions with a few tweaks to get things to pass:

1. I added the watch cleanup on destroy as we started getting lots of errors trying to set properties on destroyed objects and seemed sensible.

2. Reverted the changes to the lt-infinity element (tagName, classNames and hbs template) as (a) your example would run into issues where there is more than one light table in the document and any attempt to select the specific element caused TypeError: Invalid value used as weak map key all over the place. Reverting seemed simpler and to work.

3. I've dropped the lt-infinity rows property as I can't see that we use it anywhere, it wasn't documented, tests still pass without it and dummy app renders fine.

Final thoughts on the changes to ember-in-viewport integration @alexander-alvarez and @snewcomer ?:

1. in dropping the mixin we no longer have a classNameBinding for 'viewportEntered:in-viewport' We didn't use this internally and I can't see it being documented but it is possible that people have been using it. Should we recreate this class to avoid the breaking change?

2. Now that we're not using the mixin and are importing the service ourselves we could revert the breaking change to rename the closure action inViewport => enterViewport . I'm in favour of keeping it but we could now deprecate it nicely I suppose. Another option would be to revert the change to the lt-body component (which is documented) but leave it in lt-infinity which is not documented.

[snewcomer]

@fran-worley Very nice work! We can keep the inViewport action if we rename the service. Something like inViewportService: service('in-viewport'). Not using the template is ok too! If you found a reason you needed it, we would need something like this.

Otherwise looks great!

[fran-worley]

@alexander-alvarez , @snewcomer I believe this PR is ready to go. I'm going to raise a new one to update Ember cli etc to avoid the div getting too big.

I've added a deprecation warning to anyone using the inViewport event on lt-body component as this event is included in the current docs. I don't consider the changes to the lt-infinity component breaking as this component isn't documented and is therefore purely internal. Happy to revise if you think best.

[snewcomer]

Great work @fran-worley! Just for perpetuity sake, the bug here was in ember-in-viewport and not property "exiting" with a scrollable container defined.

Also if you wanted to avoid the deprecation, you could rename the service injection to inViewportService. Otherwise, 👍

[snewcomer]

just for thoughts

[fran-worley]

@snewcomer IMHO the deprecation to rename the action/event is better:

1. grammatically enterViewport and exitViewport are nicer than inViewport and exitViewport
2. We've already seen how people can unexpectedly run into issues because of the name overlap here - which you can probably close once this is merged!

[adong]

--scheduleScrolledToBottom: observer('rows.[]', 'isInViewport', function() {	  
++scheduleScrolledToBottom: observer('isInViewport', function() {

It indeed broke our existing functionality.

Ref: screwdriver-cd/screwdriver#2181 (comment)

---

Let me illustrate an use case that no longer worked

With rows.[], the following sequence would occur if we load 5 items at a time inside onScrolledToBottom=(action "onScrolledToBottom")

Assuming we have 10 rows in our view-port:

header
---
.
.
.
.
.
.
.
.
.
.

First time, it loads 5

header
---
row1
row2
row3
row4
row5
.
.
.
.
.

Because rows.[] changed from 0 to 5, it will continue to call and push lt-infinity out of the view-port

So we end up here, and even though rows.[] will increase 1 more time, but since its out of view-port, it will stop calling onScrolledToBottom

header
---
row1
row2
row3
row4
row5
row6
row7
row8
row9
row10

This PR removes computed property dependencies of rows.[], and broke such flow.

---

Hmm, or 🤔 maybe I'm using it wrong.