Skip to content

Commit

Permalink
fix(carousel): re-implement with committed ref
Browse files Browse the repository at this point in the history
Re-implements the reference to the interval value with a committed
ref, ensuring that the interval value we're accessing is safe
when accessing the value inside of effects.

This in turn fixes the consistency issues within our unit tests
(although I still ended up increasing the time being ticked to
give a little more leeway for the events being fired).

This also includes a re-work of traversing the children to grab
the interval values at the same time that we count the children,
removing that potential side effect from the rendering logic.
  • Loading branch information
bpas247 committed Aug 7, 2020
1 parent d4e12f0 commit c8edc2c
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 14 deletions.
28 changes: 15 additions & 13 deletions src/Carousel.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import useEventCallback from '@restart/hooks/useEventCallback';
import useUpdateEffect from '@restart/hooks/useUpdateEffect';
import useCommittedRef from '@restart/hooks/useCommittedRef';
import useTimeout from '@restart/hooks/useTimeout';
import classNames from 'classnames';
import transitionEnd from 'dom-helpers/transitionEnd';
Expand All @@ -16,7 +17,7 @@ import React, {
import { useUncontrolled } from 'uncontrollable';
import CarouselCaption from './CarouselCaption';
import CarouselItem from './CarouselItem';
import { map } from './ElementChildren';
import { map, forEach } from './ElementChildren';
import SafeAnchor from './SafeAnchor';
import { useBootstrapPrefix } from './ThemeProvider';
import triggerBrowserReflow from './triggerBrowserReflow';
Expand Down Expand Up @@ -252,6 +253,10 @@ function CarouselFunc(uncontrolledProps: CarouselProps, ref) {

const intervals = useRef<number[]>([]);

const intervalRef = useCommittedRef(
intervals.current[activeIndex as number] ?? interval,
);

if (!isSliding && activeIndex !== renderedActiveIndex) {
if (nextDirectionRef.current) {
setDirection(nextDirectionRef.current);
Expand All @@ -272,9 +277,13 @@ function CarouselFunc(uncontrolledProps: CarouselProps, ref) {
}
});

const numChildren = React.Children.toArray(children).filter(
React.isValidElement,
).length;
let numChildren = 0;
// Iterate to grab all of the children's interval values
// (and count them, too)
forEach(children, (child, index) => {
intervals.current[index] = child.props.interval as number;
numChildren++;
});

const prev = useCallback(
(event) => {
Expand Down Expand Up @@ -494,23 +503,17 @@ function CarouselFunc(uncontrolledProps: CarouselProps, ref) {
return undefined;
}

let chosenInterval = interval || undefined;

if (activeIndex !== undefined && intervals.current[activeIndex]) {
chosenInterval = intervals.current[activeIndex];
}

intervalHandleRef.current = window.setInterval(
document.visibilityState ? nextWhenVisible : next,
chosenInterval,
intervalRef.current,
);

return () => {
if (intervalHandleRef.current !== null) {
clearInterval(intervalHandleRef.current);
}
};
}, [shouldPlay, next, activeIndex, interval, nextWhenVisible]);
}, [shouldPlay, next, intervalRef, interval, nextWhenVisible]);

const indicatorOnClicks = useMemo(
() =>
Expand Down Expand Up @@ -555,7 +558,6 @@ function CarouselFunc(uncontrolledProps: CarouselProps, ref) {
<div className={`${prefix}-inner`}>
{map(children, (child, index) => {
const isActive = index === renderedActiveIndex;
intervals.current[index] = child.props.interval as number;

return slide ? (
<Transition
Expand Down
2 changes: 1 addition & 1 deletion test/CarouselSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ describe('<Carousel>', () => {
);

const total = intervals.reduce((sum, current) => sum + current, 0);
clock.tick(total * 1.1);
clock.tick(total * 1.7);

expect(onSelectSpy).to.have.been.calledThrice;
expect(onSelectSpy.firstCall).to.have.been.calledWith(1);
Expand Down

0 comments on commit c8edc2c

Please sign in to comment.