From b471914261263086ebaa34b3d9216365b405a816 Mon Sep 17 00:00:00 2001 From: Marcos Moura Date: Tue, 24 Oct 2023 19:47:27 +0200 Subject: [PATCH 1/4] fix: prevent from breaking the rule of hooks --- .../src/hooks/useMotion.test.ts | 24 +----- .../src/hooks/useMotion.ts | 76 +++---------------- 2 files changed, 11 insertions(+), 89 deletions(-) diff --git a/packages/react-components/react-motion-preview/src/hooks/useMotion.test.ts b/packages/react-components/react-motion-preview/src/hooks/useMotion.test.ts index 946bf69a713e54..a554922d4f8933 100644 --- a/packages/react-components/react-motion-preview/src/hooks/useMotion.test.ts +++ b/packages/react-components/react-motion-preview/src/hooks/useMotion.test.ts @@ -1,6 +1,6 @@ import { act, renderHook } from '@testing-library/react-hooks'; -import { useMotion, MotionOptions, MotionShorthand, getDefaultMotionState, useIsMotion } from './useMotion'; +import { useMotion, MotionOptions, MotionShorthand, getDefaultMotionState } from './useMotion'; const defaultDuration = 100; const renderHookWithRef = ( @@ -89,28 +89,6 @@ describe('useMotion', () => { expect(result.current.ref).toStrictEqual(defaultState.ref); expect(result.current.active).toStrictEqual(true); }); - - it('should show error when motion changes to a different type', () => { - const spy = jest.spyOn(console, 'error').mockImplementation(() => ({})); - let defaultMotion: MotionShorthand = getDefaultMotionState(); - const { rerender } = renderHook(() => useIsMotion(defaultMotion)); - - defaultMotion = false; - - rerender(); - expect(spy).toHaveBeenCalledTimes(1); - expect(spy).toHaveBeenCalledWith( - [ - 'useMotion: The hook needs to be called with the same typeof of shorthand on every render.', - 'This is to ensure the internal state of the hook is stable and can be used to accurately detect the motion state.', - 'Please make sure to not change the shorthand on subsequent renders or to use the hook conditionally.', - '\nCurrent shorthand:', - JSON.stringify(defaultMotion, null, 2), - '\nPrevious shorthand:', - JSON.stringify(getDefaultMotionState(), null, 2), - ].join(' '), - ); - }); }); describe('when presence is false by default', () => { diff --git a/packages/react-components/react-motion-preview/src/hooks/useMotion.ts b/packages/react-components/react-motion-preview/src/hooks/useMotion.ts index 0a5db898a9b3bc..9719f387efcc10 100644 --- a/packages/react-components/react-motion-preview/src/hooks/useMotion.ts +++ b/packages/react-components/react-motion-preview/src/hooks/useMotion.ts @@ -1,5 +1,5 @@ import * as React from 'react'; -import { useAnimationFrame, useTimeout, usePrevious, useFirstMount } from '@fluentui/react-utilities'; +import { useAnimationFrame, useTimeout, useFirstMount } from '@fluentui/react-utilities'; import { useReducedMotion } from './useReducedMotion'; import { getMotionDuration } from '../utils/dom-style'; @@ -117,9 +117,7 @@ function useMotionPresence( return; } - /* - * In case animation is disabled, we can skip the animation and go straight to the idle state. - */ + // In case animation is disabled, we can skip the animation and go straight to the idle state. if (disableAnimation) { setType(presence ? 'idle' : 'unmounted'); setActive(presence); @@ -128,22 +126,16 @@ function useMotionPresence( setType(presence ? 'entering' : 'exiting'); - /* - * If the element is not rendered, nothing to do. - */ + // If the element is not rendered, nothing to do. if (!currentElement) { return; } - /* - * Wait for the next frame to ensure the element is rendered and the animation can start. - */ + // Wait for the next frame to ensure the element is rendered and the animation can start. setAnimationFrame(() => { setActive(presence); - /* - * Wait for the next frame to ensure the animation has started. - */ + // Wait for the next frame to ensure the animation has started. setAnimationFrame(() => { const duration = getMotionDuration(currentElement); @@ -165,7 +157,8 @@ function useMotionPresence( cancelAnimationFrame(); clearAnimationTimeout(); }; - /* + + /** * Only tracks dependencies that are either not stable or are used in the callbacks * This is to avoid re-running the effect on every render, especially when the element is not rendered */ @@ -207,57 +200,8 @@ export function useMotion( shorthand: MotionShorthand, options?: MotionOptions, ): MotionState { - /** - * Heads up! - * This hook returns a Motion but also accepts Motion as an argument. - * In case the hook is called with a Motion as argument, we don't need to perform the expensive computation of the - * motion state and can just return the motion value as is. This is intentional as it allows others to use the hook - * on their side without having to worry about the performance impact of the hook. - */ - // eslint-disable-next-line react-hooks/rules-of-hooks - return useIsMotion(shorthand) ? shorthand : useMotionPresence(shorthand, options); -} + const isShorthand = typeof shorthand === 'object'; + const motion = useMotionPresence(isShorthand ? false : shorthand, options); -const stringifyShorthand = (value: MotionShorthand) => { - return JSON.stringify(value, null, 2); -}; - -/** - * @internal - * - * This method emits a warning if the hook is called with - * a different typeof of shorthand on subsequent renders, - * since this can lead breaking the rules of hooks. - * - * It also return a boolean indicating whether the shorthand is a motion object. - */ -export function useIsMotion( - shorthand: MotionShorthand, -): shorthand is MotionState { - const previousShorthand = usePrevious(shorthand); - - /** - * Heads up! - * We don't want these warnings in production even though it is against native behavior - */ - if (process.env.NODE_ENV !== 'production') { - // eslint-disable-next-line react-hooks/rules-of-hooks - React.useEffect(() => { - if (previousShorthand !== null && typeof previousShorthand !== typeof shorthand) { - // eslint-disable-next-line no-console - console.error( - [ - 'useMotion: The hook needs to be called with the same typeof of shorthand on every render.', - 'This is to ensure the internal state of the hook is stable and can be used to accurately detect the motion state.', - 'Please make sure to not change the shorthand on subsequent renders or to use the hook conditionally.', - '\nCurrent shorthand:', - stringifyShorthand(shorthand), - '\nPrevious shorthand:', - stringifyShorthand(previousShorthand), - ].join(' '), - ); - } - }, [shorthand, previousShorthand]); - } - return typeof shorthand === 'object'; + return isShorthand ? shorthand : motion; } From b06e713869e34e95b8f775e4696605f13e1907e5 Mon Sep 17 00:00:00 2001 From: Marcos Moura Date: Tue, 24 Oct 2023 20:01:01 +0200 Subject: [PATCH 2/4] fix: add missing changefile --- ...otion-preview-20cc46cf-11ec-49a9-9f37-0e1385ab3b40.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/@fluentui-react-motion-preview-20cc46cf-11ec-49a9-9f37-0e1385ab3b40.json diff --git a/change/@fluentui-react-motion-preview-20cc46cf-11ec-49a9-9f37-0e1385ab3b40.json b/change/@fluentui-react-motion-preview-20cc46cf-11ec-49a9-9f37-0e1385ab3b40.json new file mode 100644 index 00000000000000..d3455374d18e3b --- /dev/null +++ b/change/@fluentui-react-motion-preview-20cc46cf-11ec-49a9-9f37-0e1385ab3b40.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "fix: avoid breaking rule of hooks", + "packageName": "@fluentui/react-motion-preview", + "email": "marcosvmmoura@gmail.com", + "dependentChangeType": "patch" +} From de127c39a39cf7f4cb1460e2d10a984e760b5586 Mon Sep 17 00:00:00 2001 From: Marcos Moura Date: Wed, 25 Oct 2023 13:56:42 +0200 Subject: [PATCH 3/4] fix: pass sane defaults to function --- .../react-motion-preview/src/hooks/useMotion.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/react-components/react-motion-preview/src/hooks/useMotion.ts b/packages/react-components/react-motion-preview/src/hooks/useMotion.ts index 9719f387efcc10..13654584b50921 100644 --- a/packages/react-components/react-motion-preview/src/hooks/useMotion.ts +++ b/packages/react-components/react-motion-preview/src/hooks/useMotion.ts @@ -201,7 +201,10 @@ export function useMotion( options?: MotionOptions, ): MotionState { const isShorthand = typeof shorthand === 'object'; - const motion = useMotionPresence(isShorthand ? false : shorthand, options); + const motion = useMotionPresence( + isShorthand ? false : shorthand, + isShorthand ? { animateOnFirstMount: false } : options, + ); return isShorthand ? shorthand : motion; } From 4e695ac93d9cbde6a5d413b5ff8aa5053c2203fd Mon Sep 17 00:00:00 2001 From: Marcos Moura Date: Wed, 25 Oct 2023 13:57:39 +0200 Subject: [PATCH 4/4] fix: revert change --- .../react-motion-preview/src/hooks/useMotion.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/react-components/react-motion-preview/src/hooks/useMotion.ts b/packages/react-components/react-motion-preview/src/hooks/useMotion.ts index 13654584b50921..9719f387efcc10 100644 --- a/packages/react-components/react-motion-preview/src/hooks/useMotion.ts +++ b/packages/react-components/react-motion-preview/src/hooks/useMotion.ts @@ -201,10 +201,7 @@ export function useMotion( options?: MotionOptions, ): MotionState { const isShorthand = typeof shorthand === 'object'; - const motion = useMotionPresence( - isShorthand ? false : shorthand, - isShorthand ? { animateOnFirstMount: false } : options, - ); + const motion = useMotionPresence(isShorthand ? false : shorthand, options); return isShorthand ? shorthand : motion; }