makes event listener callbacks execute asynchronously

[rovolution]

Background

After diving through our source code and tests, I realized a serious issue...none of our event listeners were executing asynchronously!

Though we were exposing a callback based API, the callbacks were being executed synchronously whenever the private method _trigger() was being executed.

I wrapped the callback invocations in setTimeout() with a timeout of 0 (which cues the callback to be invoked on the next event loop tick) and fixed a bunch of our tests to reflect this.

Pull Requests

Please accompany all pull requests with the following (where appropriate):

• unit tests (we use Jasmine 2.x.x)
• JSFiddle (or an equivalent such as CodePen, Plunker, etc) or screenshot/GIF with new feature or bug-fix
• Link to original Github issue (if this is a bug-fix)
• documentation updates to README file
• examples within /tpl/index.tpl (for new options being added)
• Passes CI-server checks (runs automated tests, JS source-code linting, etc..). To run these on your local machine, type grunt test in your Terminal within the bootstrap-slider repository directory

[jespirit]

Nice work! I'm surprised you fix all the tests that were failing. I expected a lot more to fail.

[jespirit]

For future reference:

setTimeout() with delay of 0. Under Late timeouts:
https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/setTimeout#Reasons_for_delays_longer_than_specified

[rovolution]

@jespirit Thanks for the review! i updated with feedback. take a glance and let me know if it all looks good.

[jespirit]

Upon further review, I think we should revert the changes to how the callbacks are called and instead change the unit tests so they are executed asynchronously when callbacks are expected to be called.

[jespirit]

Syntax error: }, 0); added argument 0 to it() instead of window.setTimeout()

[jespirit]

$.trigger() is not async

I have removed the done argument and this test should fail because the 'slideStop' callback should be executed asynchronously. However, this test will still pass because _trigger() will call $.trigger() synchronously, and .trigger() will call custom events synchronously, with the exception of built-in events, which are executed sync/async depending on browser.

bootstrap-slider/src/js/bootstrap-slider.js

Lines 1876 to 1883 in a5fc012

	_triggerJQueryEvent: function(evt, val) {
	var eventData = {
	type: evt,
	value: val
	};
	this.$element.trigger(eventData);
	this.$sliderElem.trigger(eventData);
	},

See this thread: https://stackoverflow.com/questions/14017013/is-the-jquery-trigger-function-guaranteed-to-be-synchronous

it("'slideStop' event is triggered properly and can be binded to", function() {
  touch.initEvent("touchstop");
  testSlider.on('slideStop', function() {  // this will be called synchronously
    expect(true).toBeTruthy();
  });
  testSlider.data('slider')._mouseup(mouse);
  window.setTimeout(function() {
    expect(spy).toHaveBeenCalled();
    // done();
  }, 0);
});

Tests Should Be Async

Upon further review,

I think the issue is not that the callbacks are being executed synchronously, I think the way we had it before was fine, it's just that the tests themselves were not being executed asynchronously.

So I think the tests should be reorganized to be as follows:

// Register callbacks
testSlider.on(/* slideStart, slide, slideStop */, function() { /* code here */});

setTimeout(function() {
  // set up code here to be executed asynchronously (next tick of the event loop)
}, 0);

For example,

it("Should trigger change on mouseup", function(done) {
  var changes = 0;

  // Register callbacks
  testSlider.on('slideStop', function(){
    expect(changes).toBe(2);
    expect(testSlider.getValue()).toEqual([2, 5]);
    done();
  });
  
  testSlider.on('change', function(){
    changes++;
  });

  // Execute this code async (on next tick of the event loop)
  setTimeout(function() {
    testSlider.mousedown(mouseEvent('mousedown', tickOffsets[1]));
    expect(testSlider.getValue()).toEqual([1, 5]);

    testSlider.mouseup(mouseEvent('mouseup', tickOffsets[2]));
  }, 0);
});

If you remove the call to done() then the test will timeout.

Drag handles over each other and use keyboard to move handles over each other
     √ should drag and keydown handles properly to the right then back to the left
     √ should drag and keydown handles properly to the left then back to the right
   √ Should snap to a tick within tick bounds when using the mouse navigation
   × Should trigger change on mouseup
     Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL. (1)

[jespirit]

See my review comment below, the callbacks should be executed synchronously at the moment they're triggered, which is how $.trigger() behaves.

[rovolution]

thanks for pointing this out!

I reflected on this a bit more and I agree with you. Also, when a user triggers a mouse or keyboard based sliding event, this automatically triggers the necessary callbacks in an async fashion via the browser's native event handling mechanism.

If a user directly changes the value via the setValue() call, then the event handlers should execute in a sync fashion (as you indicated).

I looked at another popular slider library called noUiSlider, and this is how they do it as well.

[rovolution]

@jespirit just removed the changes + removed the default timeout amount of 0 in the unit tests (setTimeout defaults to 0 seconds when a specific timeout is not specified)

[jespirit]

It seems like every time I look at this pull request, there's something I missed. I left my review comments, take a look.

[rovolution]

@jespirit feel free to make the changes you want if you don't want me to block you. i probably will not have time to work on this