dont rely on didInsertElement setting up scrollable

[ghost]

In the project I'm working on currently we had problems with embedded routes and refreshing.

After refreshing the embedded route, the ember-infinity component would get re-initialized, without didInsertElement getting called. Thus the _setScrollable function was never called to set up the jquery object and we we're getting an error stating that "scrollable.height" is not a function.

I will add a test case in the course of the day to demonstrate the problem.

Cheers
Manuel Arno Korfmann

[hhff]

Thanks @mkorfmann ! I was under the impression didInsertElement was always called whenever a component is rendered! A test would def help me understand this usecase.

[ghost]

Hey @hhff, I could successfully reproduce the buggy behaviour using the latest ember 1.13.5.

The last build will probably succeed, since I didn't comment out the fix. But I will trigger one more build now and you should see at least the ember-release build failing.

[ghost]

There you go: https://travis-ci.org/hhff/ember-infinity/jobs/73547152.

To fix it, just uncomment the line I commented out in order to get the build to fail.
You can see which line I mean here: https://github.com/mkorfmann/ember-infinity/commit/9721abedb2e861763c57ca65b91a4297e0de169e.

Manuel Arno Korfmann

[ghost]

The changed behaviour of didInsertElement is documented here: http://emberjs.com/blog/2015/06/12/ember-1-13-0-released.html#toc_component-lifecycle-hooks.

The build has now succeeded, including the added test for the changed behaviour of didInsertElement.

[mike-north]

This is not the proper way to check versions.

Here's a way based on a regex

var EMBER_VERSION_REGEX = /^([0-9]+)\.([0-9]+)\.([0-9]+)(?:(?:\-(alpha|beta)\.([0-9]+)(?:\.([0-9]+))?)?)?(?:\+(canary))?(?:\.([0-9abcdef]+))?(?:\-([A-Za-z0-9\.\-]+))?(?:\+([A-Za-z0-9\.\-]+))?$/;
/**
 * VERSION_INFO[i] is as follows:
 *
 * 0  complete version string
 * 1  major version
 * 2  minor version
 * 3  trivial version
 * 4  pre-release type (optional: "alpha" or "beta" or undefined for stable releases)
 * 5  pre-release version (optional)
 * 6  pre-release sub-version (optional)
 * 7  canary (optional: "canary", or undefined for stable releases)
 * 8  SHA (optional)
 */
var VERSION_INFO = EMBER_VERSION_REGEX.exec(Ember.VERSION);

And then the check

 var isPre110 = parseInt(VERSION_INFO[1], 10) < 2 && parseInt(VERSION_INFO[2], 10) < 11;

[kellyselden]

@hhff Is this something your ember version addon would support?

[hhff]

yeah it is https://github.com/hhff/ember-version-is

[ghost]

Hey!

@mike-north: Thanks for the heads-up, I'll change this asap.

@hhff: what would be your preferred way of version checking, using the regex or your package?

Cheers

[hhff]

i'd prefer we use Ember Version Is - The idea behind it is that as we stop supporting old versions of ember, we simply go through and snip those blocks. It's more a maintainability thing than anything else.

[mike-north]

I'd be careful about using a semver based approach. Some Ember version numbers violate semver. It would be good to add some test cases to ember-version-is that represent these semver-violating strings.

[ghost]

Hey @hhff, is there still something to be done for this pull request to get merged?

[hhff]

LGTM @mkorfmann - can u squash the commits?

[kellyselden]

I would encourage a regression test for pre 1.13.0, but that could be a separate effort.

[ghost]

Squashed the commits.

@kellyselden Correct me if I don't understand your cause properly, but: The travis build is also run against Ember versions prior to 1.13.0 and the added test fails without the fix on these builds as well.

[kellyselden]

I'm unsure how old of ember builds Travis would test. When we are deep in 2.x land, will pre 1.13.0 still be tested? @hhff

[mike-north]

@kellyselden I should be done with this today ember-cli/ember-try#26 allowing addons to easily test a wide range of ember versions -- potentially with different trees of code/templates

[kellyselden]

@mike-north Awesome.
@mkorfmann This issue of how many version combos we support and how can be addressed elsewhere. This LGTM.

[cprussin]

Due to this PR, the infinity-loader _setup() method is called every time the spinner is rerendered. This means it is resetting state if it were to be removed from the page--the example we have is that the spinner is inside a collapsible accordion and when expanding, the scroller is re-initialized.

This is binding events multiple times and causing the 'scrollable' property to be set improperly (on first init, _setupScrollable points the property to the DOM element. On subsequent inits, _setupScrollable detects that the property is not a string and sets it to Em.$(window)).

My suspicion is that _setup() should not be called in didRender, but rather should be called in an init method. However, I'm fairly unfamiliar with the internals of this library and the difficulty of supporting old ember versions. Can someone more familiar chime in?

[ghost]

This can be solved with a simple check in _setupScrollable, only setting scrollable equal to Em.$(window) when it is undefined or null.

If @hhff is d'accord with this additional fix, I'm happy to request another pull with this fix.

Cheers
Manuel Arno Korfmann

[cprussin]

Ok, but what about the multiple events being bound?

Is there a good reason _setup is called outside an Ember.on('init') ?

[ghost]

If you read my first post in this pull request and the added test case, you should be able to understand the issue with not calling _setupScrollable on subsequent rerenders. If it is necessary to rebind all the events is another question. Maybe we can do it this way: Calling _setup in didInsertElement and just calling _setupScrollable without the event rebinding in didRender. Maybe we even need another test case to be sure that event rebinding is not necessary on subsequent rerenders.

[ghost]

BTW, Which version of Ember and ember-infinity were you using when it was still working?

[ghost]

Is Ember.on('init') called after the dom is ready? If it is uncertain, that the scrollable DOM-Element is already available, it won't be of much use to call _setup or _setupScrollable for that matter.

[cprussin]

It was never working properly; this is my first try at using ember-infinity.

Ember.on('init') is called before the DOM is rendered. So I'm thinking something like:

init: function() {
  Ember.run.scheduleOnce('afterRender', _setup);
}

[ghost]

I'm currently trying to reproduce the buggy behaviour you're experiencing, but so far no luck.

Could you share the bit of code responsible for the collapsing with me? Or, if you must adhere to a NDA, tell me how you would reproduce it?

[cprussin]

Sure, but it's deep in the app so I'll need to extract out a minimal example. I'll try to whip up a test case in the next few days.

[hhff]

Thanks for checking into this guys. FWIW - init is called before the dom is ready.

[ghost]

IMO, we should fix it this way: Making sure, that _setup only binds one listener for each event and _scrollable gets only re-initialized if it isn't defined already.

This way we can be sure everything works if the component is re-rendered and calling _setup on each render is going to be completely OK perfomance-wise.

I'm not really sure why we are having the issue at all, but there should be a good reason why didRender is called after route refreshes and not didInsertElement after Ember 1.13.0. If not, then this may be an issue with Ember itself.

But anyway, since this approach won't cost us any significant perfomance as far as I can tell and it works, I already implemented the changes for testing purposes: #69.

I suggest we continue discussing the issue there, since this one is already merged.