[refactor] Portal #1918
[refactor] Portal #1918
Conversation
* fix: el isn't removed in react18 strict mode develop environment * feat: make classname is reactive * feat: depracate `style` prop, add `initStyle` instead * feat: add basic test cases
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit b8638ab:
|
|
This PR is expected to be processed this week |
|
Could you give some details about why and how this PR fixed "el isn't removed in react18 strict mode develop environment" which is descripted at title? |
When you develop in react18 strict mode, the So I move 11.24.mp4 |
| } | ||
| }; | ||
| const getContainer = this.props.getPopupContainer || context?.getPopupContainer || defaultGetContainer; | ||
| const container = getContainer(); |
There was a problem hiding this comment.
Since "this.getContainer" is called at constructor and didMount, user's getPopupContainer func is called twice, this may be unexpected to user's will, beacuse user's getPopupContainer may have side effect.
There was a problem hiding this comment.
I will change the logic. But we just cannot recognize if user's getPopupContainer returns a valid node. The code could be a little weird.
There was a problem hiding this comment.
It doesn't matter, if user return a invalid dom, it's their bug, because the type definetion of getPopupContaienr declear it should return a valid dom.
The problem is we should only execute getPopupContainer once, because user's getPopupContainer may have side effect, for example, user may create a dom and append it to body and return the dom in the getPopupcontainer function, if we execute the function twice after this semi version, user's webpage will have duplicate container.
There was a problem hiding this comment.
The best solution, I think, is to make getPopupContainer reactive and mark it a break change🫣
| const container = this.getContainer(); | ||
| if (this.el.parentElement !== container) { | ||
| if (container) { | ||
| container.appendChild(this.el); |
There was a problem hiding this comment.
Delaying append "this.el" will cause trobble, since there could be some logic that user may use their real dom in their component or component'children didmount, and since appending "this.el" is delayed, user's code will crash after upgrade semi-ui version.
There was a problem hiding this comment.
Indeed, it's a tradeoff, and I'm uncertain about which way to choose.
There was a problem hiding this comment.
createPortal will fire all componentDidmount function of arg component's entire react component tree, but if we delay the dom appending operation, the component inside the portal won't have real dom on the page (the dom is created by react but not in the page) even their componentDidupdate executed. And this is very wired.
There was a problem hiding this comment.
I believe the design is sound. React components can be mounted at any node, even if that node isn't attached to a document. The root being attached to a document is no business to react component.
There was a problem hiding this comment.
In the former version of semi, we put both createPortal and insert operation all after didMount, but our company user reported that we delay the mount of their component passed to the portal, which will cause their code some bug, so we change into current code.
There was a problem hiding this comment.
I believe the design is sound. React components can be mounted at any node, even if that node isn't attached to a document. The root being attached to a document is no business to react component.
yes
There was a problem hiding this comment.
Considering React's substantial emphasis on server components, it's advisable to avoid incorporating side effects within the constructor or render methods. It's conceivable that future versions may discourage or disallow such practices.
There was a problem hiding this comment.
It's up to you to choose a proper time to make a break change. I totally understand. 🤠
There was a problem hiding this comment.
The server component is in beta for a lone time，and the promoter of this feature have leaved facebook recently， the future of server component is uncertain, we will discuss the pr at a proper time.
|
The pr will reopen when we find a way to handle all those stuff. |
|
Thanks for your contribution ❤ |
Portaland components based onPortal, such asModal/Tooltip/Popover, don't support ssr or ssg or you will encounter react error 418 or 423.And you can fix this by wrapping this component in a dynamic import. For example, you can use
next/dynamicwithssr: falsein nextjs. Here is nextjs's official doc: https://nextjs.org/docs/pages/building-your-application/optimizing/lazy-loading#with-no-ssrstyleprop, addinitStyleinstead