refactor: rely on useEffect only to trigger initial animation (#252)

Can't rule out that this commit doesn't introduce any regression bug related to the initial animation. If so, let's revert it.
toomuchdesign · Oct 30, 2022 · 376342a · 376342a
1 parent fcfd66c
commit 376342a
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 107 deletions.
diff --git a/jest/__tests__/__snapshots__/bundles-snapshot.test.js.snap b/jest/__tests__/__snapshots__/bundles-snapshot.test.js.snap
@@ -346,26 +346,6 @@ exports[`Dist bundle is unchanged 1`] = `
     return paths;
   }
 
-  function useEffectAfterFirstPaint(effect) {
-    React.useEffect(function () {
-      if (effect) {
-        var timerId;
-        var RAFId;
-        timerId = setTimeout(function () {
-          timerId = null;
-          RAFId = requestAnimationFrame(function () {
-            effect();
-            RAFId = null;
-          });
-        });
-        return function () {
-          timerId && clearTimeout(timerId);
-          RAFId && cancelAnimationFrame(RAFId);
-        };
-      }
-    }, []);
-  }
-
   var defaultProps = {
     animationDuration: 500,
     animationEasing: 'ease-out',
@@ -387,10 +367,12 @@ exports[`Dist bundle is unchanged 1`] = `
         revealOverride = _useState[0],
         setRevealOverride = _useState[1];
 
-    useEffectAfterFirstPaint(props.animate ? function () {
-      // Trigger initial animation
-      setRevealOverride(null);
-    } : undefined);
+    React.useEffect(function () {
+      if (props.animate) {
+        // Trigger initial animation
+        setRevealOverride(null);
+      }
+    }, []);
     var extendedData = extendData(props);
     return /*#__PURE__*/React__default['default'].createElement(\\"svg\\", {
       viewBox: \\"0 0 \\" + props.viewBoxSize[0] + \\" \\" + props.viewBoxSize[1],

diff --git a/src/Chart/Chart.tsx b/src/Chart/Chart.tsx
@@ -12,27 +12,6 @@ import renderSegments from './renderSegments';
 import type { Data, BaseDataEntry, LabelRenderFunction } from '../commonTypes';
 import { makePropsWithDefaults } from '../utils';
 
-function useEffectAfterFirstPaint(effect?: () => void) {
-  useEffect(() => {
-    if (effect) {
-      let timerId: NodeJS.Timeout | null;
-      let RAFId: number | null;
-      timerId = setTimeout(() => {
-        timerId = null;
-        RAFId = requestAnimationFrame(() => {
-          effect();
-          RAFId = null;
-        });
-      });
-
-      return () => {
-        timerId && clearTimeout(timerId);
-        RAFId && cancelAnimationFrame(RAFId);
-      };
-    }
-  }, []);
-}
-
 export type Props<DataEntry extends BaseDataEntry = BaseDataEntry> = {
   animate?: boolean;
   animationDuration?: number;
@@ -100,14 +79,12 @@ export function ReactMinimalPieChart<DataEntry extends BaseDataEntry>(
     props.animate ? 0 : null
   );
 
-  useEffectAfterFirstPaint(
-    props.animate
-      ? () => {
-          // Trigger initial animation
-          setRevealOverride(null);
-        }
-      : undefined
-  );
+  useEffect(() => {
+    if (props.animate) {
+      // Trigger initial animation
+      setRevealOverride(null);
+    }
+  }, []);
 
   const extendedData = extendData(props);
   return (

diff --git a/src/__tests__/Chart.test.tsx b/src/__tests__/Chart.test.tsx
@@ -3,16 +3,6 @@ import { act, render, dataMock, getArcInfo } from './testUtils';
 import { degreesToRadians, extractPercentage } from '../utils';
 import { pieChartDefaultProps } from '../../src';
 
-jest.useFakeTimers();
-
-beforeAll(() => {
-  /// @ts-expect-error: this is a partial mock
-  global.requestAnimationFrame = (callback: () => void) => {
-    callback();
-    return 'id';
-  };
-});
-
 describe('Chart', () => {
   describe('SVG root element', () => {
     it('receive className, style and children props', () => {
@@ -187,13 +177,12 @@ describe('Chart', () => {
     describe.each`
       reveal       | expectedRevealedPercentage
       ${undefined} | ${100}
-      ${25}        | ${25}
-    `('reveal === ${reveal}', ({ reveal, expectedRevealedPercentage }) => {
-      it('re-render on did mount revealing the expected portion of segment', () => {
+    `('reveal === $reveal', ({ reveal, expectedRevealedPercentage }) => {
+      it('re-renders on mount revealing the expected portion of segment', () => {
         const segmentRadius = pieChartDefaultProps.radius / 2;
         const lengthAngle = pieChartDefaultProps.lengthAngle;
         const fullPathLength = degreesToRadians(segmentRadius) * lengthAngle;
-        let hiddenPercentage;
+        let expectedHiddenPercentage;
         const initialProps = {
           data: [dataMock[0]],
           animate: true,
@@ -202,25 +191,26 @@ describe('Chart', () => {
         const { container, reRender } = render(initialProps);
         const path = container.querySelector('path');
 
-        // Paths are hidden
-        hiddenPercentage = 100;
-        expect(path).toHaveAttribute('stroke-dasharray', `${fullPathLength}`);
-        expect(path).toHaveAttribute(
-          'stroke-dashoffset',
-          `${extractPercentage(fullPathLength, hiddenPercentage)}`
-        );
-
-        // Trigger async initial animation
-        act(() => {
-          jest.runAllTimers();
-        });
+        /**
+         * @NOTE testing library fires useEffect sync and therefore we can't
+         * assert against the DOM before useEffect is executed
+         *
+         * @TODO Find a way to ensure that segments are hidden on mount:
+         *
+         * expectedHiddenPercentage = 100;
+         * expect(path).toHaveAttribute('stroke-dasharray', `${fullPathLength}`);
+         * expect(path).toHaveAttribute(
+         *   'stroke-dashoffset',
+         *   `${extractPercentage(fullPathLength, expectedHiddenPercentage)}`
+         * );
+         */
 
         // Paths are revealed
-        hiddenPercentage = 100 - expectedRevealedPercentage;
+        expectedHiddenPercentage = 100 - expectedRevealedPercentage;
         expect(path).toHaveAttribute('stroke-dasharray', `${fullPathLength}`);
         expect(path).toHaveAttribute(
           'stroke-dashoffset',
-          `${extractPercentage(fullPathLength, hiddenPercentage)}`
+          `${extractPercentage(fullPathLength, expectedHiddenPercentage)}`
         );
 
         // Update reveal prop after initial animation
@@ -230,34 +220,15 @@ describe('Chart', () => {
           reveal: newReveal,
         });
 
-        hiddenPercentage = 100 - newReveal;
+        expectedHiddenPercentage = 100 - newReveal;
         expect(path).toHaveAttribute('stroke-dasharray', `${fullPathLength}`);
         expect(path).toHaveAttribute(
           'stroke-dashoffset',
-          `${extractPercentage(fullPathLength, hiddenPercentage)}`
+          `${extractPercentage(fullPathLength, expectedHiddenPercentage)}`
         );
       });
     });
 
-    it("don't re-render when component is unmounted", () => {
-      // Simulate edge case of animation fired after component was unmounted
-      // See: https://github.com/toomuchdesign/react-minimal-pie-chart/issues/8
-      const consoleError = jest.spyOn(console, 'error');
-      const { unmount } = render({
-        animate: true,
-      });
-
-      unmount();
-      // Trigger async initial animation
-      act(() => {
-        jest.runAllTimers();
-      });
-
-      // @ts-expect-error: This is a Jest mocke
-      console.error.mockRestore();
-      expect(consoleError).not.toHaveBeenCalled();
-    });
-
     describe('stroke-dashoffset attribute', () => {
       it("doesn't generate zero rounding issues after animation (GitHub: #133)", () => {
         const { container } = render({
@@ -268,11 +239,6 @@ describe('Chart', () => {
           animate: true,
         });
 
-        // Trigger async initial animation
-        act(() => {
-          jest.runAllTimers();
-        });
-
         // Expect all segments to be fully exposed
         container.querySelectorAll('path').forEach((path) => {
           expect(path).toHaveAttribute('stroke-dashoffset', '0');