feat: getContainer改变后更新Portal的容器 #89
Conversation
binyellow
commented
Mar 22, 2020
binyellow
commented
- 修复的问题现象是：当我改变容器后，visible变化第一次不会渲染到对的container中，当visible第二次变为true才会渲染正确
- 问题的原因是：当getContainer改变后，内部createPortal的container一直没变，和这个类似#17645
- removeChild的节点的引用会被存起来，因为引用指向父组件的this.container，但是该节点和父子节点的关联关系被删除掉了，导致下次渲染在PortalWrapper中判断存在，不会重新createElement，所以我在unMount中将container置为null了。参考
|
This pull request introduces 1 alert when merging fe8155a into 9453a84 - view on LGTM.com new alerts:
|
src/Portal.js
Outdated
| @@ -25,19 +29,23 @@ export default class Portal extends React.Component { | |||
| } | |||
|
|
|||
| createContainer() { | |||
| this._container = this.props.getContainer(); | |||
| this.portalContainer = this.props.getContainer(); | |||
src/Portal.js
Outdated
| if (this._container) { | ||
| this._container.parentNode.removeChild(this._container); | ||
| if (this.portalContainer) { | ||
| this.portalContainer = null; |
There was a problem hiding this comment.
这个 null 没看懂，为啥要改，如果是 container 变了的原因多加个判断就行
tests/container.test.js
Outdated
| {() => 'Content'} | ||
| </PortalWrapper>, | ||
| ); | ||
| expect(wrapper).toMatchSnapshot(); |
There was a problem hiding this comment.
package.json
Outdated
| @@ -44,6 +44,7 @@ | |||
| "create-react-class": "^15.6.3", | |||
| "enzyme": "^3.10.0", | |||
| "eslint": "^6.6.0", | |||
| "eslint-plugin-react-hooks": "^2.5.1", | |||
tests/container.test.js
Outdated
| getContainer: document.getElementById('dom1'), | ||
| }); | ||
| const wrapperDom1 = portal.parent(); | ||
| expect(oldWrapper !== wrapperDom1).toEqual(true); |
There was a problem hiding this comment.
有点点小问题，mount 返回的是个 wrap 对象，你这个等于 if( {} !== {} )，可以这样写测试
describe('container', () => {
let div1
let div2
beforeAll(() => {
div1 = document.createElement('div');
div1.id = 'dom1';
div2 = document.createElement('div');
div2.id = 'dom2';
document.body.appendChild(div1);
document.body.appendChild(div2);
});
afterAll(() => {
document.body.removeChild(div1);
document.body.removeChild(div2);
})
it('Same function returns different DOM', async () => {
const wrapper = mount(
<PortalWrapper
visible
getContainer={() => document.getElementById('dom1')}
>
{() => <div id="children">Content</div>}
</PortalWrapper>,
);
expect(document.querySelector('#dom1 #children')).not.toBeNull()
wrapper.setProps({ getContainer: () => document.getElementById('dom2') });
expect(document.querySelector('#dom1 #children')).toBeNull()
expect(document.querySelector('#dom2 #children')).not.toBeNull()
});
});There was a problem hiding this comment.
恩 去antd里面也找了例子，没找到类似测wrapper得，本来这里用contains，发现可能是因为position为absolute，导致一直返回false，就不知道咋写，谢谢学习了
src/Portal.js
Outdated
| const currentContainer = this.props.getContainer(); | ||
| if (currentContainer && this._container !== currentContainer) { | ||
| this._container = currentContainer; | ||
| } |
There was a problem hiding this comment.
这个最好可以放到 lifeCycle 里面去，参考 wrapper，就这样吧
|
这么一改每次 render 都要更新一下。比较好奇什么需求会需要动态改容器？ |
我们有个需求是要根据用户的配置去改变抽屉的容器。只会在container变化才会更新this._container |
|
个人是不太想支持这个更新，对于所有依赖了它的组件都会有性能损耗。而得到的好处也并不太多。从业务角度也是可以通过全刷新来实现更新容器的。 你们什么想法？ @afc163 @shaodahong @AshoneA |
|
感觉可以把 container 放到组件内部解决？ https://codesandbox.io/s/anniuleixing-ant-design-demo-v7dtx 我觉得用外部的一个 mutable 的变量来决定 React 视图其实不太好，毕竟 React 更加推崇用 v = f(d) 来决定视图。如果实在需要依赖外部用户配置，我觉得业务需求上应该想想怎么在用户配置改变的时候去更新下组件内的状态，因为如果你业务上组件依赖外部 mutable 变量（用户配置），估计其他地方也会碰到一些 React 视图不更新的问题。 |
| @@ -37,6 +37,10 @@ export default class Portal extends React.Component { | |||
|
|
|||
| render() { | |||
| if (this._container) { | |||
| const currentContainer = this.props.getContainer(); | |||
| if (currentContainer && this._container !== currentContainer) { | |||
| this._container = currentContainer; | |||
There was a problem hiding this comment.
| this._container = currentContainer; | |
| this.removeContainer(); | |
| this._container = currentContainer; |
There was a problem hiding this comment.
恩 是的，测到了。
关于更新的问题，其实PortalWrapper里面也做了相应的处理，在getDerivedStateFromProps中做了判断和移除操作，只不过Portal内没更新，Portal不做更新复用之前的container在性能上确实会好点，看你们判断吧
