Skip to content

Commit

Permalink
fix: ensure all intervals/timeouts/observers are cleared when compone…
Browse files Browse the repository at this point in the history
…nt is destroyed (#5362)

* chore: tests cleanup

* Only stub components when really needed

* Update button-toolbar.spec.js

* Update carousel-slide.spec.js

* Update form-checkbox-group.spec.js

* Update form-radio-group.spec.js

* Update tabs.spec.js

* Update click-out.spec.js

* Update focus-in.spec.js

* Update dom.spec.js

* fix: ensure all intervals/timeouts/observers are cleared

* Unify variable name for non-reactive properties

* Shave off some bytes

* Update visible.js

* Update mixin-tbody.js

* Update modal.js

* Update carousel.js

Co-authored-by: Troy Morehouse <troymore@nbnet.nb.ca>
  • Loading branch information
jacobmllr95 and tmorehouse committed May 14, 2020
1 parent c3db758 commit 064cdf4
Show file tree
Hide file tree
Showing 14 changed files with 137 additions and 134 deletions.
70 changes: 40 additions & 30 deletions src/components/carousel/carousel.js
Original file line number Diff line number Diff line change
Expand Up @@ -205,9 +205,10 @@ export const BCarousel = /*#__PURE__*/ Vue.extend({
},
created() {
// Create private non-reactive props
this._intervalId = null
this._animationTimeout = null
this._touchTimeout = null
this.$_interval = null
this.$_animationTimeout = null
this.$_touchTimeout = null
this.$_observer = null
// Set initial paused state
this.isPaused = !(toInteger(this.interval, 0) > 0)
},
Expand All @@ -217,22 +218,39 @@ export const BCarousel = /*#__PURE__*/ Vue.extend({
// Get all slides
this.updateSlides()
// Observe child changes so we can update slide list
observeDom(this.$refs.inner, this.updateSlides.bind(this), {
subtree: false,
childList: true,
attributes: true,
attributeFilter: ['id']
})
this.setObserver(true)
},
beforeDestroy() {
clearTimeout(this._animationTimeout)
clearTimeout(this._touchTimeout)
clearInterval(this._intervalId)
this._intervalId = null
this._animationTimeout = null
this._touchTimeout = null
this.clearInterval()
this.clearAnimationTimeout()
this.clearTouchTimeout()
this.setObserver(false)
},
methods: {
clearInterval() {
clearInterval(this.$_interval)
this.$_interval = null
},
clearAnimationTimeout() {
clearTimeout(this.$_animationTimeout)
this.$_animationTimeout = null
},
clearTouchTimeout() {
clearTimeout(this.$_touchTimeout)
this.$_touchTimeout = null
},
setObserver(on = false) {
this.$_observer && this.$_observer.disconnect()
this.$_observer = null
if (on) {
this.$_observer = observeDom(this.$refs.inner, this.updateSlides.bind(this), {
subtree: false,
childList: true,
attributes: true,
attributeFilter: ['id']
})
}
},
// Set slide
setSlide(slide, direction = null) {
// Don't animate when page is not visible
Expand Down Expand Up @@ -286,24 +304,18 @@ export const BCarousel = /*#__PURE__*/ Vue.extend({
if (!evt) {
this.isPaused = true
}
if (this._intervalId) {
clearInterval(this._intervalId)
this._intervalId = null
}
this.clearInterval()
},
// Start auto rotate slides
start(evt) {
if (!evt) {
this.isPaused = false
}
/* istanbul ignore next: most likely will never happen, but just in case */
if (this._intervalId) {
clearInterval(this._intervalId)
this._intervalId = null
}
this.clearInterval()
// Don't start if no interval, or less than 2 slides
if (this.interval && this.numSlides > 1) {
this._intervalId = setInterval(this.next, mathMax(1000, this.interval))
this.$_interval = setInterval(this.next, mathMax(1000, this.interval))
}
},
// Restart auto rotate slides when focus/hover leaves the carousel
Expand Down Expand Up @@ -362,7 +374,7 @@ export const BCarousel = /*#__PURE__*/ Vue.extend({
eventOff(currentSlide, evt, onceTransEnd, EVENT_OPTIONS_NO_CAPTURE)
)
}
this._animationTimeout = null
this.clearAnimationTimeout()
removeClass(nextSlide, dirClass)
removeClass(nextSlide, overlayClass)
addClass(nextSlide, 'active')
Expand All @@ -387,7 +399,7 @@ export const BCarousel = /*#__PURE__*/ Vue.extend({
)
}
// Fallback to setTimeout()
this._animationTimeout = setTimeout(onceTransEnd, TRANS_DURATION)
this.$_animationTimeout = setTimeout(onceTransEnd, TRANS_DURATION)
}
if (isCycling) {
this.start(false)
Expand Down Expand Up @@ -480,10 +492,8 @@ export const BCarousel = /*#__PURE__*/ Vue.extend({
// is NOT fired) and after a timeout (to allow for mouse compatibility
// events to fire) we explicitly restart cycling
this.pause(false)
if (this._touchTimeout) {
clearTimeout(this._touchTimeout)
}
this._touchTimeout = setTimeout(
this.clearTouchTimeout()
this.$_touchTimeout = setTimeout(
this.start,
TOUCH_EVENT_COMPAT_WAIT + mathMax(1000, this.interval)
)
Expand Down
2 changes: 2 additions & 0 deletions src/components/form-spinbutton/form-spinbutton.js
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,8 @@ export const BFormSpinbutton = /*#__PURE__*/ Vue.extend({
resetTimers() {
clearTimeout(this.$_autoDelayTimer)
clearInterval(this.$_autoRepeatTimer)
this.$_autoDelayTimer = null
this.$_autoRepeatTimer = null
},
clearRepeat() {
this.resetTimers()
Expand Down
31 changes: 16 additions & 15 deletions src/components/modal/modal.js
Original file line number Diff line number Diff line change
Expand Up @@ -450,7 +450,7 @@ export const BModal = /*#__PURE__*/ Vue.extend({
},
created() {
// Define non-reactive properties
this._observer = null
this.$_observer = null
},
mounted() {
// Set initial z-index as queried from the DOM
Expand All @@ -470,17 +470,25 @@ export const BModal = /*#__PURE__*/ Vue.extend({
},
beforeDestroy() {
// Ensure everything is back to normal
if (this._observer) {
this._observer.disconnect()
this._observer = null
}
this.setObserver(false)
if (this.isVisible) {
this.isVisible = false
this.isShow = false
this.isTransitioning = false
}
},
methods: {
setObserver(on = false) {
this.$_observer && this.$_observer.disconnect()
this.$_observer = null
if (on) {
this.$_observer = observeDom(
this.$refs.content,
this.checkModalOverflow.bind(this),
OBSERVER_CONFIG
)
}
},
// Private method to update the v-model
updateModel(val) {
if (val !== this.visible) {
Expand Down Expand Up @@ -562,10 +570,7 @@ export const BModal = /*#__PURE__*/ Vue.extend({
return
}
// Stop observing for content changes
if (this._observer) {
this._observer.disconnect()
this._observer = null
}
this.setObserver(false)
// Trigger the hide transition
this.isVisible = false
// Update the v-model
Expand Down Expand Up @@ -615,13 +620,9 @@ export const BModal = /*#__PURE__*/ Vue.extend({
// Update the v-model
this.updateModel(true)
this.$nextTick(() => {
// In a nextTick in case modal content is lazy
// Observe changes in modal content and adjust if necessary
this._observer = observeDom(
this.$refs.content,
this.checkModalOverflow.bind(this),
OBSERVER_CONFIG
)
// In a `$nextTick()` in case modal content is lazy
this.setObserver(true)
})
})
},
Expand Down
15 changes: 8 additions & 7 deletions src/components/table/helpers/mixin-filtering.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,7 @@ export default {
// Watch for debounce being set to 0
computedFilterDebounce(newVal) {
if (!newVal && this.$_filterTimer) {
clearTimeout(this.$_filterTimer)
this.$_filterTimer = null
this.clearFilterTimer()
this.localFilter = this.filterSanitize(this.filter)
}
},
Expand All @@ -113,8 +112,7 @@ export default {
deep: true,
handler(newCriteria) {
const timeout = this.computedFilterDebounce
clearTimeout(this.$_filterTimer)
this.$_filterTimer = null
this.clearFilterTimer()
if (timeout && timeout > 0) {
// If we have a debounce time, delay the update of `localFilter`
this.$_filterTimer = setTimeout(() => {
Expand Down Expand Up @@ -155,7 +153,7 @@ export default {
}
},
created() {
// Create non-reactive prop where we store the debounce timer id
// Create private non-reactive props
this.$_filterTimer = null
// If filter is "pre-set", set the criteria
// This will trigger any watchers/dependents
Expand All @@ -167,10 +165,13 @@ export default {
})
},
beforeDestroy() /* istanbul ignore next */ {
clearTimeout(this.$_filterTimer)
this.$_filterTimer = null
this.clearFilterTimer()
},
methods: {
clearFilterTimer() {
clearTimeout(this.$_filterTimer)
this.$_filterTimer = null
},
filterSanitize(criteria) {
// Sanitizes filter criteria based on internal or external filtering
if (
Expand Down
3 changes: 3 additions & 0 deletions src/components/table/helpers/mixin-tbody.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ const props = {
export default {
mixins: [tbodyRowMixin],
props,
beforeDestroy() {
this.$_bodyFieldSlotNameCache = null
},
methods: {
// Helper methods
getTbodyTrs() {
Expand Down
17 changes: 6 additions & 11 deletions src/components/tabs/tabs.js
Original file line number Diff line number Diff line change
Expand Up @@ -312,8 +312,9 @@ export const BTabs = /*#__PURE__*/ Vue.extend({
}
},
created() {
// Create private non-reactive props
this.$_observer = null
this.currentTab = toInteger(this.value, -1)
this._bvObserver = null
// For SSR and to make sure only a single tab is shown on mount
// We wrap this in a `$nextTick()` to ensure the child tabs have been created
this.$nextTick(() => {
Expand Down Expand Up @@ -362,11 +363,11 @@ export const BTabs = /*#__PURE__*/ Vue.extend({
unregisterTab(tab) {
this.registeredTabs = this.registeredTabs.slice().filter(t => t !== tab)
},
// DOM observer is needed to detect changes in order of tabs
setObserver(on) {
// DOM observer is needed to detect changes in order of tabs
this.$_observer && this.$_observer.disconnect()
this.$_observer = null
if (on) {
// Make sure no existing observer running
this.setObserver(false)
const self = this
/* istanbul ignore next: difficult to test mutation observer in JSDOM */
const handler = () => {
Expand All @@ -379,18 +380,12 @@ export const BTabs = /*#__PURE__*/ Vue.extend({
})
}
// Watch for changes to <b-tab> sub components
this._bvObserver = observeDom(this.$refs.tabsContainer, handler, {
this.$_observer = observeDom(this.$refs.tabsContainer, handler, {
childList: true,
subtree: false,
attributes: true,
attributeFilter: ['id']
})
} else {
/* istanbul ignore next */
if (this._bvObserver && this._bvObserver.disconnect) {
this._bvObserver.disconnect()
}
this._bvObserver = null
}
},
getTabs() {
Expand Down
10 changes: 5 additions & 5 deletions src/components/tooltip/helpers/bv-popper.js
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,10 @@ export const BVPopper = /*#__PURE__*/ Vue.extend({
updated() {
// Update popper if needed
// TODO: Should this be a watcher on `this.popperConfig` instead?
this.popperUpdate()
this.updatePopper()
},
beforeDestroy() {
this.popperDestroy()
this.destroyPopper()
},
destroyed() {
// Make sure template is removed from DOM
Expand Down Expand Up @@ -198,16 +198,16 @@ export const BVPopper = /*#__PURE__*/ Vue.extend({
return this.offset
},
popperCreate(el) {
this.popperDestroy()
this.destroyPopper()
// We use `el` rather than `this.$el` just in case the original
// mountpoint root element type was changed by the template
this.$_popper = new Popper(this.target, el, this.popperConfig)
},
popperDestroy() {
destroyPopper() {
this.$_popper && this.$_popper.destroy()
this.$_popper = null
},
popperUpdate() {
updatePopper() {
this.$_popper && this.$_popper.scheduleUpdate()
},
popperPlacementChange(data) {
Expand Down
14 changes: 5 additions & 9 deletions src/components/tooltip/helpers/bv-tooltip.js
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ export const BVTooltip = /*#__PURE__*/ Vue.extend({
this.clearActiveTriggers()
this.localPlacementTarget = null
try {
this.$_tip && this.$_tip.$destroy()
this.$_tip.$destroy()
} catch {}
this.$_tip = null
this.removeAriaDescribedby()
Expand Down Expand Up @@ -552,16 +552,12 @@ export const BVTooltip = /*#__PURE__*/ Vue.extend({
return this.isDropdown() && target && select(DROPDOWN_OPEN_SELECTOR, target)
},
clearHoverTimeout() {
if (this.$_hoverTimeout) {
clearTimeout(this.$_hoverTimeout)
this.$_hoverTimeout = null
}
clearTimeout(this.$_hoverTimeout)
this.$_hoverTimeout = null
},
clearVisibilityInterval() {
if (this.$_visibleInterval) {
clearInterval(this.$_visibleInterval)
this.$_visibleInterval = null
}
clearInterval(this.$_visibleInterval)
this.$_visibleInterval = null
},
clearActiveTriggers() {
for (const trigger in this.activeTrigger) {
Expand Down
Loading

0 comments on commit 064cdf4

Please sign in to comment.