Skip to content

Commit

Permalink
fix(carousel): setInterval memory leak when no slides provided (#2399)
Browse files Browse the repository at this point in the history
  • Loading branch information
tmorehouse authored Jan 6, 2019
1 parent f46258f commit ac2a708
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 5 deletions.
9 changes: 5 additions & 4 deletions src/components/carousel/carousel.js
Original file line number Diff line number Diff line change
Expand Up @@ -213,8 +213,7 @@ export default {
attributeFilter: [ 'id' ]
})
},
/* istanbul ignore next: dificult to test */
beforeDestroy () {
beforeDestroy () /* istanbul ignore next: dificult to test */ {
clearInterval(this.intervalId)
clearTimeout(this._animationTimeout)
this.intervalId = null
Expand All @@ -224,6 +223,7 @@ export default {
// Set slide
setSlide (slide) {
// Don't animate when page is not visible
/* istanbul ignore if: dificult to test */
if (typeof document !== 'undefined' && document.visibilityState && document.hidden) {
return
}
Expand Down Expand Up @@ -266,8 +266,8 @@ export default {
},
// Start auto rotate slides
start () {
// Don't start if no interval, or if we are already running
if (!this.interval || this.isCycling) {
// Don't start if no interval, no slides, or if we are already running
if (!this.interval || this.isCycling || this.intervalId || this.slides.length === 0) {
return
}
this.slides.forEach(slide => {
Expand All @@ -279,6 +279,7 @@ export default {
},
// Re-Start auto rotate slides when focus/hover leaves the carousel
restart (evt) {
/* istanbul ignore if: dificult to test */
if (!this.$el.contains(document.activeElement)) {
this.start()
}
Expand Down
25 changes: 25 additions & 0 deletions src/components/carousel/carousel.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,4 +70,29 @@ describe('carousel', async () => {
expect(carousel.isSliding).toBe(false)
})
})

it('Should scroll to specified slide', async () => {
const { app } = window
const carousel = app.$refs.carousel

const spyBegin = jest.fn()
const spyEnd = jest.fn()

carousel.$on('sliding-start', spyBegin)
carousel.$on('sliding-end', spyEnd)

app.slide = 2

app.$nextTick(() => {
expect(spyBegin).toHaveBeenCalled()
expect(carousel.isSliding).toBe(true)
})

jest.runAllTimers()

app.$nextTick(() => {
expect(spyEnd).toHaveBeenCalledWith(app.slide)
expect(carousel.isSliding).toBe(false)
})
})
})
10 changes: 9 additions & 1 deletion src/components/carousel/fixtures/carousel.html
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,16 @@ <h1>Hello world!</h1>

</b-carousel>

<p class="mt-4">
<p class="mt-4 mb-4">
Slide #: {{ slide }}<br>
Is Sliding: {{ sliding }}
</p>

<!-- empty carousel -->
<b-carousel id="carousel2"
ref="carousel2"
controls
indicators
background="grey">
</b-carousel>
</div>

0 comments on commit ac2a708

Please sign in to comment.