From b13f0e9520ea0fdd54ea201afe0b4ec25f53169a Mon Sep 17 00:00:00 2001 From: Oleksandr Fediashov Date: Fri, 3 Jan 2020 17:05:15 +0100 Subject: [PATCH 1/4] fix(useStateManager): keep reference to `actions` --- .../src/hooks/useStateManager.ts | 15 +++++++-- .../test/hooks/useStateManager-test.tsx | 31 ++++++++++++++++++- 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/packages/react-bindings/src/hooks/useStateManager.ts b/packages/react-bindings/src/hooks/useStateManager.ts index 9ab42d4ea4..02e1e1e11f 100644 --- a/packages/react-bindings/src/hooks/useStateManager.ts +++ b/packages/react-bindings/src/hooks/useStateManager.ts @@ -36,6 +36,7 @@ const useStateManager = < mapPropsToState = () => ({} as Partial), sideEffects = [], } = options + const latestActions = React.useMemo(() => ({} as Actions), [managerFactory]) const latestManager = React.useRef | null>(null) // Heads up! forceUpdate() is used only for triggering rerenders, stateManager is SSOT @@ -58,19 +59,29 @@ const useStateManager = < ], }) + // We need to keep the same reference to an object with actions to allow usage them as + // a dependency in useCallback() hook + Object.assign(latestActions, latestManager.current.actions) + + // For development environments we disallow ability to extend object with other properties to + // avoid misusage + if (process.env.NODE_ENV !== 'production') { + if (Object.isExtensible(latestActions)) Object.preventExtensions(latestActions) + } + // We need to pass exactly `manager.state` to provide the same state object during the same render // frame. // It keeps behavior consistency between React state tools and our managers // https://github.com/facebook/react/issues/11527#issuecomment-360199710 if (process.env.NODE_ENV === 'production') { - return { state: latestManager.current.state, actions: latestManager.current.actions } + return { state: latestManager.current.state, actions: latestActions } } // Object.freeze() is used only in dev-mode to avoid usage mistakes return { state: Object.freeze(latestManager.current.state), - actions: Object.freeze(latestManager.current.actions), + actions: latestActions, } } diff --git a/packages/react-bindings/test/hooks/useStateManager-test.tsx b/packages/react-bindings/test/hooks/useStateManager-test.tsx index f39e597616..bbbd57046e 100644 --- a/packages/react-bindings/test/hooks/useStateManager-test.tsx +++ b/packages/react-bindings/test/hooks/useStateManager-test.tsx @@ -1,6 +1,6 @@ import { useStateManager } from '@fluentui/react-bindings' import { createManager, ManagerFactory } from '@fluentui/state' -import { shallow } from 'enzyme' +import { mount, shallow } from 'enzyme' import * as React from 'react' import * as ReactTestUtils from 'react-dom/test-utils' @@ -63,6 +63,22 @@ const TestComponent: React.FunctionComponent = props => { ) } +type ActionsComponentProps = { + onRender: () => void + onUpdate: () => void +} + +const ActionsComponent: React.FunctionComponent = props => { + const { actions } = useStateManager(createTestManager) + + props.onRender() + React.useEffect(() => { + props.onUpdate() + }, [actions]) + + return
actions.toggle()} /> +} + describe('useStateManager', () => { it('uses default values from state manager', () => { const wrapper = shallow() @@ -148,4 +164,17 @@ describe('useStateManager', () => { expect(onChange).toHaveBeenNthCalledWith(1, 'foo') expect(onChange).toHaveBeenNthCalledWith(2, 'foo') }) + + it('actions are referentially equal between renders', () => { + const onRender = jest.fn() + const onUpdate = jest.fn() + const wrapper = mount() + + ReactTestUtils.act(() => { + wrapper.find('div').simulate('click') + }) + + expect(onRender).toHaveBeenCalledTimes(2) + expect(onUpdate).toHaveBeenCalledTimes(1) + }) }) From 8dc585dd422f07be6ec3d232e278f9646ced388e Mon Sep 17 00:00:00 2001 From: Oleksandr Fediashov Date: Wed, 12 Feb 2020 14:55:05 +0100 Subject: [PATCH 2/4] fix(useStateManager): keep reference to `actions` --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c90135ed1d..43f2c4a862 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Fix page crash when `Tooltip` content is null @delprzemo ([#2332](https://github.com/microsoft/fluent-ui-react/pull/2332)) - Fix `document` usage in `mergeProviderContexts` to get SSR working @layershifter ([#2330](https://github.com/microsoft/fluent-ui-react/pull/2330)) - Fix `input` descenders being cropped in the Teams theme @bcalvery ([#2335](https://github.com/microsoft/fluent-ui-react/pull/2335)) +- Use referentially equal objects for `actions` in `useStateManager` @layershifter ([#2347](https://github.com/microsoft/fluent-ui-react/pull/2347)) ### Features - Added sourcemaps to the dist output to simplify debugging @miroslavstastny ([#2329](https://github.com/microsoft/fluent-ui-react/pull/2329)) From 939ffd0666805356b903561ef57bedbc344adb9e Mon Sep 17 00:00:00 2001 From: Oleksandr Fediashov Date: Wed, 12 Feb 2020 15:07:42 +0100 Subject: [PATCH 3/4] improve UTs --- .../test/hooks/useStateManager-test.tsx | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/packages/react-bindings/test/hooks/useStateManager-test.tsx b/packages/react-bindings/test/hooks/useStateManager-test.tsx index bbbd57046e..737638658c 100644 --- a/packages/react-bindings/test/hooks/useStateManager-test.tsx +++ b/packages/react-bindings/test/hooks/useStateManager-test.tsx @@ -69,14 +69,15 @@ type ActionsComponentProps = { } const ActionsComponent: React.FunctionComponent = props => { - const { actions } = useStateManager(createTestManager) + const { actions, state } = useStateManager(createTestManager) + const handleClick = React.useCallback(() => actions.toggle(), [actions]) props.onRender() React.useEffect(() => { props.onUpdate() }, [actions]) - return
actions.toggle()} /> + return
} describe('useStateManager', () => { @@ -170,11 +171,24 @@ describe('useStateManager', () => { const onUpdate = jest.fn() const wrapper = mount() + expect(wrapper.find('div').prop('data-open')).toBe(false) + ReactTestUtils.act(() => { wrapper.find('div').simulate('click') }) + wrapper.update() + expect(wrapper.find('div').prop('data-open')).toBe(true) expect(onRender).toHaveBeenCalledTimes(2) expect(onUpdate).toHaveBeenCalledTimes(1) + + ReactTestUtils.act(() => { + wrapper.find('div').simulate('click') + }) + wrapper.update() + + expect(wrapper.find('div').prop('data-open')).toBe(false) + expect(onRender).toHaveBeenCalledTimes(3) + expect(onUpdate).toHaveBeenCalledTimes(1) }) }) From 5742d764044549ef32a5864f752f7744c0177bbb Mon Sep 17 00:00:00 2001 From: Oleksandr Fediashov Date: Wed, 12 Feb 2020 16:47:40 +0100 Subject: [PATCH 4/4] merge to one statement --- packages/react-bindings/src/hooks/useStateManager.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/react-bindings/src/hooks/useStateManager.ts b/packages/react-bindings/src/hooks/useStateManager.ts index 02e1e1e11f..035eee16a3 100644 --- a/packages/react-bindings/src/hooks/useStateManager.ts +++ b/packages/react-bindings/src/hooks/useStateManager.ts @@ -73,14 +73,13 @@ const useStateManager = < // frame. // It keeps behavior consistency between React state tools and our managers // https://github.com/facebook/react/issues/11527#issuecomment-360199710 - - if (process.env.NODE_ENV === 'production') { - return { state: latestManager.current.state, actions: latestActions } - } - // Object.freeze() is used only in dev-mode to avoid usage mistakes + return { - state: Object.freeze(latestManager.current.state), + state: + process.env.NODE_ENV === 'production' + ? latestManager.current.state + : Object.freeze(latestManager.current.state), actions: latestActions, } }