From 29e98f61d058f2d8f99ff1850531b6e09a6328fb Mon Sep 17 00:00:00 2001 From: zombiej Date: Mon, 28 Sep 2020 11:46:12 +0800 Subject: [PATCH 1/6] refactor: Clean up unecessary code --- src/ContainerRender.js | 4 +++ src/PortalWrapper.tsx | 70 +++++++++--------------------------------- 2 files changed, 18 insertions(+), 56 deletions(-) diff --git a/src/ContainerRender.js b/src/ContainerRender.js index 462876f3..f4e0bba2 100644 --- a/src/ContainerRender.js +++ b/src/ContainerRender.js @@ -1,6 +1,10 @@ import React from 'react'; import ReactDOM from 'react-dom'; +/** + * @deprecated Since we do not need support React15 any more. + * Will remove in next major version. + */ export default class ContainerRender extends React.Component { static defaultProps = { autoMount: true, diff --git a/src/PortalWrapper.tsx b/src/PortalWrapper.tsx index 9711b475..dc6105c5 100644 --- a/src/PortalWrapper.tsx +++ b/src/PortalWrapper.tsx @@ -1,27 +1,20 @@ /* eslint-disable no-underscore-dangle,react/require-default-props */ import * as React from 'react'; -import * as ReactDOM from 'react-dom'; import raf from './raf'; -import ContainerRender from './ContainerRender'; import Portal, { PortalRef } from './Portal'; import switchScrollingEffect from './switchScrollingEffect'; import setStyle from './setStyle'; +import canUseDom from './Dom/canUseDom'; let openCount = 0; -const windowIsUndefined = !( - typeof window !== 'undefined' && - window.document && - window.document.createElement -); - -const IS_REACT_16 = 'createPortal' in ReactDOM; +const supportDom = canUseDom(); // https://github.com/ant-design/ant-design/issues/19340 // https://github.com/ant-design/ant-design/issues/19332 let cacheOverflow = {}; const getParent = (getContainer: GetContainer) => { - if (windowIsUndefined) { + if (!supportDom) { return null; } if (getContainer) { @@ -66,7 +59,7 @@ class PortalWrapper extends React.Component< > { container?: HTMLElement; - _component?: PortalRef; + component?: PortalRef; rafId?: number; @@ -81,7 +74,7 @@ class PortalWrapper extends React.Component< constructor(props: PortalWrapperProps) { super(props); const { visible, getContainer } = props; - if (!windowIsUndefined && getParent(getContainer) === document.body) { + if (supportDom && getParent(getContainer) === document.body) { openCount = visible ? openCount + 1 : openCount; } this.state = { @@ -104,11 +97,11 @@ class PortalWrapper extends React.Component< componentWillUnmount() { const { visible, getContainer } = this.props; - if (!windowIsUndefined && getParent(getContainer) === document.body) { + if (supportDom && getParent(getContainer) === document.body) { // 离开时不会 render, 导到离开时数值不变,改用 func 。。 openCount = visible && openCount ? openCount - 1 : openCount; } - this.removeCurrentContainer(visible); + this.removeCurrentContainer(); raf.cancel(this.rafId); } @@ -121,7 +114,7 @@ class PortalWrapper extends React.Component< } = prevProps; if ( visible !== prevVisible && - !windowIsUndefined && + supportDom && getParent(getContainer) === document.body ) { openCount = visible && !prevVisible ? openCount + 1 : openCount - 1; @@ -157,7 +150,7 @@ class PortalWrapper extends React.Component< }; getContainer = () => { - if (windowIsUndefined) { + if (!supportDom) { return null; } if (!this.container) { @@ -182,23 +175,12 @@ class PortalWrapper extends React.Component< savePortal = (c: PortalRef) => { // Warning: don't rename _component // https://github.com/react-component/util/pull/65#discussion_r352407916 - this._component = c; + this.component = c; }; - removeCurrentContainer = visible => { + removeCurrentContainer = () => { this.container = null; - this._component = null; - if (!IS_REACT_16) { - if (visible) { - this.renderComponent({ - afterClose: this.removeContainer, - onClose() {}, - visible: false, - }); - } else { - this.removeContainer(); - } - } + this.component = null; }; /** @@ -233,32 +215,8 @@ class PortalWrapper extends React.Component< getContainer: this.getContainer, switchScrollingEffect: this.switchScrollingEffect, }; - // support react15 - if (!IS_REACT_16) { - return ( - - children({ - ...extra, - ...childProps, - ref: this.savePortal, - }) - } - getContainer={this.getContainer} - forceRender={forceRender} - > - {({ renderComponent, removeContainer }) => { - this.renderComponent = renderComponent; - this.removeContainer = removeContainer; - return null; - }} - - ); - } - if (forceRender || visible || this._component) { + + if (forceRender || visible || this.component) { portal = ( {children(childProps)} From 2e09248a600e71e8ef86ef3eab37e56858311d0e Mon Sep 17 00:00:00 2001 From: zombiej Date: Mon, 28 Sep 2020 11:54:03 +0800 Subject: [PATCH 2/6] remove useless code --- src/PortalWrapper.tsx | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/src/PortalWrapper.tsx b/src/PortalWrapper.tsx index dc6105c5..96edf32b 100644 --- a/src/PortalWrapper.tsx +++ b/src/PortalWrapper.tsx @@ -59,7 +59,7 @@ class PortalWrapper extends React.Component< > { container?: HTMLElement; - component?: PortalRef; + componentRef: React.RefObject = React.createRef(); rafId?: number; @@ -69,8 +69,6 @@ class PortalWrapper extends React.Component< visible: boolean; }) => void; - removeContainer?: Function; - constructor(props: PortalWrapperProps) { super(props); const { visible, getContainer } = props; @@ -127,7 +125,7 @@ class PortalWrapper extends React.Component< ? getContainer.toString() !== prevGetContainer.toString() : getContainer !== prevGetContainer ) { - _self.removeCurrentContainer(false); + _self.removeCurrentContainer(); } } return { @@ -172,15 +170,8 @@ class PortalWrapper extends React.Component< } }; - savePortal = (c: PortalRef) => { - // Warning: don't rename _component - // https://github.com/react-component/util/pull/65#discussion_r352407916 - this.component = c; - }; - removeCurrentContainer = () => { - this.container = null; - this.component = null; + this.container.parentNode?.removeChild(this.container); }; /** @@ -216,9 +207,9 @@ class PortalWrapper extends React.Component< switchScrollingEffect: this.switchScrollingEffect, }; - if (forceRender || visible || this.component) { + if (forceRender || visible || this.componentRef.current) { portal = ( - + {children(childProps)} ); From 0664a681d60ed688be5932edbb44953b4ab5046e Mon Sep 17 00:00:00 2001 From: zombiej Date: Mon, 28 Sep 2020 13:01:05 +0800 Subject: [PATCH 3/6] fix test --- src/Portal.tsx | 11 ++--------- src/PortalWrapper.tsx | 4 +++- tests/Portal.test.tsx | 2 +- 3 files changed, 6 insertions(+), 11 deletions(-) diff --git a/src/Portal.tsx b/src/Portal.tsx index 05f77baf..b602f575 100644 --- a/src/Portal.tsx +++ b/src/Portal.tsx @@ -1,10 +1,5 @@ import * as React from 'react'; -import { - useRef, - useEffect, - forwardRef, - useImperativeHandle, -} from 'react'; +import { useRef, useEffect, forwardRef, useImperativeHandle } from 'react'; import ReactDOM from 'react-dom'; import canUseDom from './Dom/canUseDom'; @@ -38,9 +33,7 @@ const Portal = forwardRef((props, ref) => { useEffect(() => { return () => { - if (containerRef.current) { - containerRef.current.parentNode.removeChild(containerRef.current); - } + containerRef.current?.parentNode?.removeChild(containerRef.current); }; }, []); diff --git a/src/PortalWrapper.tsx b/src/PortalWrapper.tsx index 96edf32b..524a27d6 100644 --- a/src/PortalWrapper.tsx +++ b/src/PortalWrapper.tsx @@ -171,7 +171,9 @@ class PortalWrapper extends React.Component< }; removeCurrentContainer = () => { - this.container.parentNode?.removeChild(this.container); + // Portal will remove from `parentNode`. + // Let's handle this again to avoid refactor issue. + this.container?.parentNode?.removeChild(this.container); }; /** diff --git a/tests/Portal.test.tsx b/tests/Portal.test.tsx index 92d5cf62..88c810a6 100644 --- a/tests/Portal.test.tsx +++ b/tests/Portal.test.tsx @@ -15,7 +15,7 @@ describe('Portal', () => { document.body.removeChild(container); }); - it('forceRender', () => { + it.only('forceRender', () => { const divRef = React.createRef(); const wrapper = mount( From 12243227d6550dd14c1f02a1cf62ee4bd817dbdb Mon Sep 17 00:00:00 2001 From: zombiej Date: Mon, 28 Sep 2020 13:04:32 +0800 Subject: [PATCH 4/6] add comment --- src/Portal.tsx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Portal.tsx b/src/Portal.tsx index b602f575..9aaf1c66 100644 --- a/src/Portal.tsx +++ b/src/Portal.tsx @@ -33,6 +33,8 @@ const Portal = forwardRef((props, ref) => { useEffect(() => { return () => { + // [Legacy] This should not be handle by Portal but parent PortalWrapper instead. + // Since some component use `Portal` directly, we have to keep the logic here. containerRef.current?.parentNode?.removeChild(containerRef.current); }; }, []); From 460b079bd1fbfd16289133482d2875f689616e76 Mon Sep 17 00:00:00 2001 From: zombiej Date: Mon, 28 Sep 2020 13:05:00 +0800 Subject: [PATCH 5/6] clean up --- tests/Portal.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Portal.test.tsx b/tests/Portal.test.tsx index 88c810a6..92d5cf62 100644 --- a/tests/Portal.test.tsx +++ b/tests/Portal.test.tsx @@ -15,7 +15,7 @@ describe('Portal', () => { document.body.removeChild(container); }); - it.only('forceRender', () => { + it('forceRender', () => { const divRef = React.createRef(); const wrapper = mount( From 0cce39f43c100c66ffdc3206abe0d7adefce6839 Mon Sep 17 00:00:00 2001 From: zombiej Date: Mon, 28 Sep 2020 13:44:20 +0800 Subject: [PATCH 6/6] test coverage --- src/PortalWrapper.tsx | 5 ++ tests/Portal.test.tsx | 110 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 114 insertions(+), 1 deletion(-) diff --git a/src/PortalWrapper.tsx b/src/PortalWrapper.tsx index 524a27d6..e3b63598 100644 --- a/src/PortalWrapper.tsx +++ b/src/PortalWrapper.tsx @@ -9,6 +9,11 @@ import canUseDom from './Dom/canUseDom'; let openCount = 0; const supportDom = canUseDom(); +/** @private Test usage only */ +export function getOpenCount() { + return process.env.NODE_ENV === 'test' ? openCount : 0; +} + // https://github.com/ant-design/ant-design/issues/19340 // https://github.com/ant-design/ant-design/issues/19332 let cacheOverflow = {}; diff --git a/tests/Portal.test.tsx b/tests/Portal.test.tsx index 92d5cf62..fb14ed5b 100644 --- a/tests/Portal.test.tsx +++ b/tests/Portal.test.tsx @@ -1,11 +1,16 @@ import React from 'react'; import { mount } from 'enzyme'; -import PortalWrapper from '../src/PortalWrapper'; +import { act } from 'react-dom/test-utils'; +import PortalWrapper, { getOpenCount } from '../src/PortalWrapper'; import Portal from '../src/Portal'; describe('Portal', () => { let container: HTMLDivElement; + // Mock for raf + window.requestAnimationFrame = callback => window.setTimeout(callback); + window.cancelAnimationFrame = id => window.clearTimeout(id); + beforeEach(() => { container = document.createElement('div'); document.body.appendChild(container); @@ -46,4 +51,107 @@ describe('Portal', () => { wrapper.setProps({ justForceUpdate: true }); expect(didUpdate).toHaveBeenCalledTimes(2); }); + + describe('getContainer', () => { + it('string', () => { + const div = document.createElement('div'); + div.id = 'bamboo-light'; + document.body.appendChild(div); + + mount( + + {() =>
2333
} +
, + ); + + expect(document.querySelector('#bamboo-light').childElementCount).toEqual( + 1, + ); + + document.body.removeChild(div); + }); + + it('function', () => { + const div = document.createElement('div'); + + mount( + div}> + {() =>
2333
} +
, + ); + + expect(div.childElementCount).toEqual(1); + }); + + it('htmlElement', () => { + const div = document.createElement('div'); + + mount( + + {() =>
2333
} +
, + ); + + expect(div.childElementCount).toEqual(1); + }); + + it('delay', () => { + jest.useFakeTimers(); + const divRef = React.createRef(); + const wrapper = mount( +
+ divRef.current}> + {() =>
} + +
+
, + ); + + act(() => { + jest.runAllTimers(); + wrapper.update(); + }); + + expect(divRef.current.childElementCount).toEqual(1); + jest.useRealTimers(); + }); + }); + + it('openCount', () => { + const Demo = ({ count, visible }: { count: number; visible: boolean }) => { + return ( + <> + {new Array(count).fill(null).map((_, index) => ( + + {() =>
2333
} +
+ ))} + + ); + }; + + const wrapper = mount(); + expect(getOpenCount()).toEqual(1); + + wrapper.setProps({ count: 2 }); + expect(getOpenCount()).toEqual(2); + + wrapper.setProps({ count: 1 }); + expect(getOpenCount()).toEqual(1); + + wrapper.setProps({ visible: false }); + expect(getOpenCount()).toEqual(0); + }); + + it('wrapperClassName', () => { + const wrapper = mount( + + {() =>
} + , + ); + expect((wrapper.instance() as any).container.className).toEqual('bamboo'); + + wrapper.setProps({ wrapperClassName: 'light' }); + expect((wrapper.instance() as any).container.className).toEqual('light'); + }); });