From 1d738d1902e15d9624407b6eab2364ad6129ced4 Mon Sep 17 00:00:00 2001 From: MadCcc <1075746765@qq.com> Date: Mon, 27 Dec 2021 18:06:46 +0800 Subject: [PATCH 1/6] fix: not allow setState in Popconfirm if unmounted --- components/_util/ActionButton.tsx | 9 +++++++-- components/popconfirm/index.tsx | 13 ++++++++++++- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/components/_util/ActionButton.tsx b/components/_util/ActionButton.tsx index 7c88bfcfd1..c3da552e8c 100644 --- a/components/_util/ActionButton.tsx +++ b/components/_util/ActionButton.tsx @@ -32,6 +32,7 @@ const ActionButton: React.FC = props => { if (timeoutId) { clearTimeout(timeoutId); } + ref.current = null; }; }, []); @@ -43,7 +44,9 @@ const ActionButton: React.FC = props => { setLoading(true); returnValueOfOnOk!.then( (...args: any[]) => { - setLoading(false); + if (ref.current) { + setLoading(false); + } close(...args); clickedRef.current = false; }, @@ -52,7 +55,9 @@ const ActionButton: React.FC = props => { // eslint-disable-next-line no-console console.error(e); // See: https://github.com/ant-design/ant-design/issues/6183 - setLoading(false); + if (ref.current) { + setLoading(false); + } clickedRef.current = false; }, ); diff --git a/components/popconfirm/index.tsx b/components/popconfirm/index.tsx index 1537e84eda..6940c85648 100644 --- a/components/popconfirm/index.tsx +++ b/components/popconfirm/index.tsx @@ -48,11 +48,22 @@ const Popconfirm = React.forwardRef((props, ref) => { defaultValue: props.defaultVisible, }); + const mounted = React.useRef(true); + + React.useEffect(() => { + mounted.current = true; + return () => { + mounted.current = false; + }; + }, []); + const settingVisible = ( value: boolean, e?: React.MouseEvent | React.KeyboardEvent, ) => { - setVisible(value); + if (mounted.current) { + setVisible(value); + } props.onVisibleChange?.(value, e); }; From 727e394d753c3dcf8c9ff5d9632c05ad8a9674b6 Mon Sep 17 00:00:00 2001 From: MadCcc <1075746765@qq.com> Date: Tue, 28 Dec 2021 17:21:00 +0800 Subject: [PATCH 2/6] feat: add useMountedRef hook --- components/_util/ActionButton.tsx | 7 ++--- .../_util/__tests__/useMountedRef.test.js | 20 ++++++++++++++ components/_util/hooks/useMountedRef.ts | 13 ++++++++++ components/popconfirm/index.tsx | 26 +++++++------------ 4 files changed, 47 insertions(+), 19 deletions(-) create mode 100644 components/_util/__tests__/useMountedRef.test.js create mode 100644 components/_util/hooks/useMountedRef.ts diff --git a/components/_util/ActionButton.tsx b/components/_util/ActionButton.tsx index c3da552e8c..8ef9fdbc77 100644 --- a/components/_util/ActionButton.tsx +++ b/components/_util/ActionButton.tsx @@ -1,6 +1,7 @@ import * as React from 'react'; import Button from '../button'; import { LegacyButtonType, ButtonProps, convertLegacyProps } from '../button/button'; +import useMountedRef from './hooks/useMountedRef'; export interface ActionButtonProps { type?: LegacyButtonType; @@ -20,6 +21,7 @@ function isThenable(thing?: PromiseLike): boolean { const ActionButton: React.FC = props => { const clickedRef = React.useRef(false); const ref = React.useRef(); + const mountedRef = useMountedRef(); const [loading, setLoading] = React.useState(false); React.useEffect(() => { @@ -32,7 +34,6 @@ const ActionButton: React.FC = props => { if (timeoutId) { clearTimeout(timeoutId); } - ref.current = null; }; }, []); @@ -44,7 +45,7 @@ const ActionButton: React.FC = props => { setLoading(true); returnValueOfOnOk!.then( (...args: any[]) => { - if (ref.current) { + if (mountedRef.current) { setLoading(false); } close(...args); @@ -55,7 +56,7 @@ const ActionButton: React.FC = props => { // eslint-disable-next-line no-console console.error(e); // See: https://github.com/ant-design/ant-design/issues/6183 - if (ref.current) { + if (mountedRef.current) { setLoading(false); } clickedRef.current = false; diff --git a/components/_util/__tests__/useMountedRef.test.js b/components/_util/__tests__/useMountedRef.test.js new file mode 100644 index 0000000000..7eb5257bca --- /dev/null +++ b/components/_util/__tests__/useMountedRef.test.js @@ -0,0 +1,20 @@ +import { mount } from 'enzyme'; +import React from 'react'; +import useMountedRef from '../hooks/useMountedRef'; + +describe('useMountedRef', () => { + it('should work properly', () => { + let ref = null; + + const AutoUnmounted = () => { + ref = useMountedRef(); + + return
Mounted
; + }; + + const wrapper = mount(); + expect(ref.current).toBeTruthy(); + wrapper.unmount(); + expect(ref.current).toBeFalsy(); + }); +}); diff --git a/components/_util/hooks/useMountedRef.ts b/components/_util/hooks/useMountedRef.ts new file mode 100644 index 0000000000..40c80f29a2 --- /dev/null +++ b/components/_util/hooks/useMountedRef.ts @@ -0,0 +1,13 @@ +import * as React from 'react'; + +export default function useMountedRef() { + const mountedRef = React.useRef(true); + + React.useEffect(() => { + return () => { + mountedRef.current = false; + }; + }, []); + + return mountedRef; +} diff --git a/components/popconfirm/index.tsx b/components/popconfirm/index.tsx index 6940c85648..2d5f27532e 100644 --- a/components/popconfirm/index.tsx +++ b/components/popconfirm/index.tsx @@ -1,18 +1,19 @@ -import * as React from 'react'; +import ExclamationCircleFilled from '@ant-design/icons/ExclamationCircleFilled'; import classNames from 'classnames'; import useMergedState from 'rc-util/lib/hooks/useMergedState'; -import ExclamationCircleFilled from '@ant-design/icons/ExclamationCircleFilled'; import KeyCode from 'rc-util/lib/KeyCode'; -import Tooltip, { AbstractTooltipProps } from '../tooltip'; +import * as React from 'react'; import Button from '../button'; -import { LegacyButtonType, ButtonProps, convertLegacyProps } from '../button/button'; +import { ButtonProps, convertLegacyProps, LegacyButtonType } from '../button/button'; +import { ConfigContext } from '../config-provider'; import LocaleReceiver from '../locale-provider/LocaleReceiver'; import defaultLocale from '../locale/default'; -import { ConfigContext } from '../config-provider'; +import Tooltip, { AbstractTooltipProps } from '../tooltip'; +import ActionButton from '../_util/ActionButton'; import { getRenderPropValue, RenderFunction } from '../_util/getRenderPropValue'; -import { cloneElement } from '../_util/reactNode'; +import useMountedRef from '../_util/hooks/useMountedRef'; import { getTransitionName } from '../_util/motion'; -import ActionButton from '../_util/ActionButton'; +import { cloneElement } from '../_util/reactNode'; export interface PopconfirmProps extends AbstractTooltipProps { title: React.ReactNode | RenderFunction; @@ -48,20 +49,13 @@ const Popconfirm = React.forwardRef((props, ref) => { defaultValue: props.defaultVisible, }); - const mounted = React.useRef(true); - - React.useEffect(() => { - mounted.current = true; - return () => { - mounted.current = false; - }; - }, []); + const mountedRef = useMountedRef(); const settingVisible = ( value: boolean, e?: React.MouseEvent | React.KeyboardEvent, ) => { - if (mounted.current) { + if (mountedRef.current) { setVisible(value); } From 19e4b578d9f3af451d09eec99b59f16b323bd171 Mon Sep 17 00:00:00 2001 From: MadCcc <1075746765@qq.com> Date: Tue, 28 Dec 2021 17:23:43 +0800 Subject: [PATCH 3/6] chore: revert import order change --- components/popconfirm/index.tsx | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/components/popconfirm/index.tsx b/components/popconfirm/index.tsx index 2d5f27532e..39467466b0 100644 --- a/components/popconfirm/index.tsx +++ b/components/popconfirm/index.tsx @@ -1,19 +1,19 @@ -import ExclamationCircleFilled from '@ant-design/icons/ExclamationCircleFilled'; +import * as React from 'react'; import classNames from 'classnames'; import useMergedState from 'rc-util/lib/hooks/useMergedState'; +import ExclamationCircleFilled from '@ant-design/icons/ExclamationCircleFilled'; import KeyCode from 'rc-util/lib/KeyCode'; -import * as React from 'react'; +import Tooltip, { AbstractTooltipProps } from '../tooltip'; import Button from '../button'; -import { ButtonProps, convertLegacyProps, LegacyButtonType } from '../button/button'; -import { ConfigContext } from '../config-provider'; +import { LegacyButtonType, ButtonProps, convertLegacyProps } from '../button/button'; import LocaleReceiver from '../locale-provider/LocaleReceiver'; import defaultLocale from '../locale/default'; -import Tooltip, { AbstractTooltipProps } from '../tooltip'; -import ActionButton from '../_util/ActionButton'; +import { ConfigContext } from '../config-provider'; import { getRenderPropValue, RenderFunction } from '../_util/getRenderPropValue'; -import useMountedRef from '../_util/hooks/useMountedRef'; -import { getTransitionName } from '../_util/motion'; import { cloneElement } from '../_util/reactNode'; +import { getTransitionName } from '../_util/motion'; +import ActionButton from '../_util/ActionButton'; +import useMountedRef from '../_util/hooks/useMountedRef'; export interface PopconfirmProps extends AbstractTooltipProps { title: React.ReactNode | RenderFunction; From 6d098783fcd44a5b2eb7da509e38bbce59d9f108 Mon Sep 17 00:00:00 2001 From: MadCcc <1075746765@qq.com> Date: Wed, 29 Dec 2021 11:31:50 +0800 Subject: [PATCH 4/6] test: add test for react memory leak warning in Popconfirm --- components/_util/hooks/useMountedRef.ts | 9 ++-- components/popconfirm/__tests__/index.test.js | 42 +++++++++++++++++++ components/popconfirm/index.tsx | 1 - 3 files changed, 47 insertions(+), 5 deletions(-) diff --git a/components/_util/hooks/useMountedRef.ts b/components/_util/hooks/useMountedRef.ts index 40c80f29a2..134f358c74 100644 --- a/components/_util/hooks/useMountedRef.ts +++ b/components/_util/hooks/useMountedRef.ts @@ -3,11 +3,12 @@ import * as React from 'react'; export default function useMountedRef() { const mountedRef = React.useRef(true); - React.useEffect(() => { - return () => { + React.useEffect( + () => () => { mountedRef.current = false; - }; - }, []); + }, + [], + ); return mountedRef; } diff --git a/components/popconfirm/__tests__/index.test.js b/components/popconfirm/__tests__/index.test.js index f0698fb16d..f704e74310 100644 --- a/components/popconfirm/__tests__/index.test.js +++ b/components/popconfirm/__tests__/index.test.js @@ -5,6 +5,7 @@ import Popconfirm from '..'; import mountTest from '../../../tests/shared/mountTest'; import { sleep } from '../../../tests/utils'; import rtlTest from '../../../tests/shared/rtlTest'; +import Button from '../../button'; describe('Popconfirm', () => { mountTest(Popconfirm); @@ -223,4 +224,45 @@ describe('Popconfirm', () => { triggerNode.simulate('keydown', { key: 'Escape', keyCode: 27 }); expect(onVisibleChange).toHaveBeenLastCalledWith(false, eventObject); }); + + it('should not warn memory leaking if setState in async callback', async () => { + const error = jest.spyOn(console, 'error'); + + const Test = () => { + const [show, setShow] = React.useState(true); + + if (show) { + return ( + + new Promise(resolve => { + setTimeout(() => { + setShow(false); + resolve(); + }, 300); + }) + } + > + + + ); + } + return ; + }; + + const wrapper = mount( +
+ +
, + ); + + expect(wrapper.text()).toEqual('Test'); + const triggerNode = wrapper.find('.clickTarget').at(0); + triggerNode.simulate('click'); + wrapper.find('.ant-btn-primary').simulate('click'); + await sleep(500); + expect(wrapper.text()).toEqual('Unmounted'); + expect(error).not.toHaveBeenCalled(); + }); }); diff --git a/components/popconfirm/index.tsx b/components/popconfirm/index.tsx index 39467466b0..039e029ff6 100644 --- a/components/popconfirm/index.tsx +++ b/components/popconfirm/index.tsx @@ -58,7 +58,6 @@ const Popconfirm = React.forwardRef((props, ref) => { if (mountedRef.current) { setVisible(value); } - props.onVisibleChange?.(value, e); }; From 285e2bd9ea1969ae4638497fe8db867a5195ddda Mon Sep 17 00:00:00 2001 From: MadCcc <1075746765@qq.com> Date: Wed, 29 Dec 2021 14:42:00 +0800 Subject: [PATCH 5/6] refactor: refactor useMountedRef to useMounted --- components/_util/ActionButton.tsx | 8 ++++---- .../{useMountedRef.test.js => useMounted.test.js} | 12 ++++++------ .../_util/hooks/{useMountedRef.ts => useMounted.ts} | 4 ++-- components/popconfirm/index.tsx | 6 +++--- 4 files changed, 15 insertions(+), 15 deletions(-) rename components/_util/__tests__/{useMountedRef.test.js => useMounted.test.js} (54%) rename components/_util/hooks/{useMountedRef.ts => useMounted.ts} (70%) diff --git a/components/_util/ActionButton.tsx b/components/_util/ActionButton.tsx index 8ef9fdbc77..feddabc2dd 100644 --- a/components/_util/ActionButton.tsx +++ b/components/_util/ActionButton.tsx @@ -1,7 +1,7 @@ import * as React from 'react'; import Button from '../button'; import { LegacyButtonType, ButtonProps, convertLegacyProps } from '../button/button'; -import useMountedRef from './hooks/useMountedRef'; +import useMounted from './hooks/useMounted'; export interface ActionButtonProps { type?: LegacyButtonType; @@ -21,7 +21,7 @@ function isThenable(thing?: PromiseLike): boolean { const ActionButton: React.FC = props => { const clickedRef = React.useRef(false); const ref = React.useRef(); - const mountedRef = useMountedRef(); + const isMounted = useMounted(); const [loading, setLoading] = React.useState(false); React.useEffect(() => { @@ -45,7 +45,7 @@ const ActionButton: React.FC = props => { setLoading(true); returnValueOfOnOk!.then( (...args: any[]) => { - if (mountedRef.current) { + if (isMounted()) { setLoading(false); } close(...args); @@ -56,7 +56,7 @@ const ActionButton: React.FC = props => { // eslint-disable-next-line no-console console.error(e); // See: https://github.com/ant-design/ant-design/issues/6183 - if (mountedRef.current) { + if (isMounted()) { setLoading(false); } clickedRef.current = false; diff --git a/components/_util/__tests__/useMountedRef.test.js b/components/_util/__tests__/useMounted.test.js similarity index 54% rename from components/_util/__tests__/useMountedRef.test.js rename to components/_util/__tests__/useMounted.test.js index 7eb5257bca..7b68df87a7 100644 --- a/components/_util/__tests__/useMountedRef.test.js +++ b/components/_util/__tests__/useMounted.test.js @@ -1,20 +1,20 @@ import { mount } from 'enzyme'; import React from 'react'; -import useMountedRef from '../hooks/useMountedRef'; +import useMounted from '../hooks/useMounted'; -describe('useMountedRef', () => { +describe('useMounted', () => { it('should work properly', () => { - let ref = null; + let isMounted = null; const AutoUnmounted = () => { - ref = useMountedRef(); + isMounted = useMounted(); return
Mounted
; }; const wrapper = mount(); - expect(ref.current).toBeTruthy(); + expect(isMounted()).toBeTruthy(); wrapper.unmount(); - expect(ref.current).toBeFalsy(); + expect(isMounted()).toBeFalsy(); }); }); diff --git a/components/_util/hooks/useMountedRef.ts b/components/_util/hooks/useMounted.ts similarity index 70% rename from components/_util/hooks/useMountedRef.ts rename to components/_util/hooks/useMounted.ts index 134f358c74..46bd267ef8 100644 --- a/components/_util/hooks/useMountedRef.ts +++ b/components/_util/hooks/useMounted.ts @@ -1,6 +1,6 @@ import * as React from 'react'; -export default function useMountedRef() { +export default function useMounted() { const mountedRef = React.useRef(true); React.useEffect( @@ -10,5 +10,5 @@ export default function useMountedRef() { [], ); - return mountedRef; + return () => mountedRef.current; } diff --git a/components/popconfirm/index.tsx b/components/popconfirm/index.tsx index 039e029ff6..13a5167991 100644 --- a/components/popconfirm/index.tsx +++ b/components/popconfirm/index.tsx @@ -13,7 +13,7 @@ import { getRenderPropValue, RenderFunction } from '../_util/getRenderPropValue' import { cloneElement } from '../_util/reactNode'; import { getTransitionName } from '../_util/motion'; import ActionButton from '../_util/ActionButton'; -import useMountedRef from '../_util/hooks/useMountedRef'; +import useMounted from '../_util/hooks/useMounted'; export interface PopconfirmProps extends AbstractTooltipProps { title: React.ReactNode | RenderFunction; @@ -49,13 +49,13 @@ const Popconfirm = React.forwardRef((props, ref) => { defaultValue: props.defaultVisible, }); - const mountedRef = useMountedRef(); + const isMounted = useMounted(); const settingVisible = ( value: boolean, e?: React.MouseEvent | React.KeyboardEvent, ) => { - if (mountedRef.current) { + if (isMounted()) { setVisible(value); } props.onVisibleChange?.(value, e); From 3effa515728661bab9a8bd743bc2f98e1d144c19 Mon Sep 17 00:00:00 2001 From: MadCcc <1075746765@qq.com> Date: Wed, 29 Dec 2021 17:10:16 +0800 Subject: [PATCH 6/6] refactor: refactor useMounted to useDestroyed --- components/_util/ActionButton.tsx | 8 ++++---- .../{useMounted.test.js => useDestroyed.test.js} | 10 +++++----- .../_util/hooks/{useMounted.ts => useDestroyed.ts} | 4 ++-- components/popconfirm/index.tsx | 6 +++--- 4 files changed, 14 insertions(+), 14 deletions(-) rename components/_util/__tests__/{useMounted.test.js => useDestroyed.test.js} (59%) rename components/_util/hooks/{useMounted.ts => useDestroyed.ts} (69%) diff --git a/components/_util/ActionButton.tsx b/components/_util/ActionButton.tsx index feddabc2dd..50540b3c55 100644 --- a/components/_util/ActionButton.tsx +++ b/components/_util/ActionButton.tsx @@ -1,7 +1,7 @@ import * as React from 'react'; import Button from '../button'; import { LegacyButtonType, ButtonProps, convertLegacyProps } from '../button/button'; -import useMounted from './hooks/useMounted'; +import useDestroyed from './hooks/useDestroyed'; export interface ActionButtonProps { type?: LegacyButtonType; @@ -21,7 +21,7 @@ function isThenable(thing?: PromiseLike): boolean { const ActionButton: React.FC = props => { const clickedRef = React.useRef(false); const ref = React.useRef(); - const isMounted = useMounted(); + const isDestroyed = useDestroyed(); const [loading, setLoading] = React.useState(false); React.useEffect(() => { @@ -45,7 +45,7 @@ const ActionButton: React.FC = props => { setLoading(true); returnValueOfOnOk!.then( (...args: any[]) => { - if (isMounted()) { + if (!isDestroyed()) { setLoading(false); } close(...args); @@ -56,7 +56,7 @@ const ActionButton: React.FC = props => { // eslint-disable-next-line no-console console.error(e); // See: https://github.com/ant-design/ant-design/issues/6183 - if (isMounted()) { + if (!isDestroyed()) { setLoading(false); } clickedRef.current = false; diff --git a/components/_util/__tests__/useMounted.test.js b/components/_util/__tests__/useDestroyed.test.js similarity index 59% rename from components/_util/__tests__/useMounted.test.js rename to components/_util/__tests__/useDestroyed.test.js index 7b68df87a7..d98979e49f 100644 --- a/components/_util/__tests__/useMounted.test.js +++ b/components/_util/__tests__/useDestroyed.test.js @@ -1,20 +1,20 @@ import { mount } from 'enzyme'; import React from 'react'; -import useMounted from '../hooks/useMounted'; +import useDestroyed from '../hooks/useDestroyed'; describe('useMounted', () => { it('should work properly', () => { - let isMounted = null; + let isDestroyed = null; const AutoUnmounted = () => { - isMounted = useMounted(); + isDestroyed = useDestroyed(); return
Mounted
; }; const wrapper = mount(); - expect(isMounted()).toBeTruthy(); + expect(isDestroyed()).toBeFalsy(); wrapper.unmount(); - expect(isMounted()).toBeFalsy(); + expect(isDestroyed()).toBeTruthy(); }); }); diff --git a/components/_util/hooks/useMounted.ts b/components/_util/hooks/useDestroyed.ts similarity index 69% rename from components/_util/hooks/useMounted.ts rename to components/_util/hooks/useDestroyed.ts index 46bd267ef8..4b361a78ae 100644 --- a/components/_util/hooks/useMounted.ts +++ b/components/_util/hooks/useDestroyed.ts @@ -1,6 +1,6 @@ import * as React from 'react'; -export default function useMounted() { +export default function useDestroyed() { const mountedRef = React.useRef(true); React.useEffect( @@ -10,5 +10,5 @@ export default function useMounted() { [], ); - return () => mountedRef.current; + return () => !mountedRef.current; } diff --git a/components/popconfirm/index.tsx b/components/popconfirm/index.tsx index 13a5167991..24cffb7940 100644 --- a/components/popconfirm/index.tsx +++ b/components/popconfirm/index.tsx @@ -13,7 +13,7 @@ import { getRenderPropValue, RenderFunction } from '../_util/getRenderPropValue' import { cloneElement } from '../_util/reactNode'; import { getTransitionName } from '../_util/motion'; import ActionButton from '../_util/ActionButton'; -import useMounted from '../_util/hooks/useMounted'; +import useDestroyed from '../_util/hooks/useDestroyed'; export interface PopconfirmProps extends AbstractTooltipProps { title: React.ReactNode | RenderFunction; @@ -49,13 +49,13 @@ const Popconfirm = React.forwardRef((props, ref) => { defaultValue: props.defaultVisible, }); - const isMounted = useMounted(); + const isDestroyed = useDestroyed(); const settingVisible = ( value: boolean, e?: React.MouseEvent | React.KeyboardEvent, ) => { - if (isMounted()) { + if (!isDestroyed()) { setVisible(value); } props.onVisibleChange?.(value, e);