Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prevent from creating multi portals #292

Merged
merged 3 commits into from
Apr 12, 2022
Merged

fix: prevent from creating multi portals #292

merged 3 commits into from
Apr 12, 2022

Conversation

tmkx
Copy link
Contributor

@tmkx tmkx commented Apr 11, 2022

@@ -858,10 +860,13 @@ export function generateTrigger(
let portal: React.ReactElement;
// prevent unmounting after it's rendered
if (popupVisible || this.popupRef.current || forceRender) {
if (!this.portalContainer) {
this.portalContainer = this.getContainer();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this also execute multiple time as FC first render?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

render will be double-invoked, but if (!this.portalContainer) can ensure getContainer is called only once.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So class component will be the same instance but FC will not. Right?

Copy link
Contributor Author

@tmkx tmkx Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So are you going to rewrite Portal to a class component? 😂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noop. Just confirm of this : )

Pls patch a comment here to tell that refactor will FC will break the behavior which helps refactor in future.

Copy link
Contributor Author

@tmkx tmkx Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, maybe not fully understood.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean comment this line: https://github.com/react-component/trigger/pull/292/files#diff-0b5adbfe7b36e4ae2f479291e20152e33e940f7f265162d77f40f6bdb5da7405R865

For example:

In React.StrictMode component will call render multiple time in first mount. When you want to refactor with FC, useRef will also init multiple time and point to different useRef instance which will create multiple element (This multiple render will not trigger effect so you can not clean up this in effect). But this is safe with class component since it always point to same class instance.

Copy link
Contributor Author

@tmkx tmkx Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, nice clear

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it's the same as FC in some ways.

class component behavior:

constructor r3mjrhp00qn
constructor 7df2h5d7k9e
render 7df2h5d7k9e
render 7df2h5d7k9e

just because that we call getContainer in render (which happened in second one, 7df2h5d7k9e).

@codecov
Copy link

codecov bot commented Apr 12, 2022

Codecov Report

Merging #292 (3c42158) into master (ac05814) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #292      +/-   ##
==========================================
+ Coverage   89.36%   89.42%   +0.05%     
==========================================
  Files          11       11              
  Lines         536      539       +3     
  Branches      137      138       +1     
==========================================
+ Hits          479      482       +3     
  Misses         57       57              
Impacted Files Coverage Δ
src/index.tsx 86.78% <100.00%> (+0.11%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@zombieJ zombieJ merged commit 03d0ad0 into react-component:master Apr 12, 2022
@tmkx tmkx deleted the fix/portal-container branch April 12, 2022 03:38
@zombieJ
Copy link
Member

zombieJ commented Apr 12, 2022

@tmkx
Copy link
Contributor Author

tmkx commented Apr 12, 2022

I'll take a look.

@zombieJ
Copy link
Member

zombieJ commented Apr 12, 2022

Let me revert this first.

zombieJ added a commit that referenced this pull request Apr 12, 2022
zombieJ added a commit that referenced this pull request Apr 12, 2022
@tmkx
Copy link
Contributor Author

tmkx commented Apr 12, 2022

This patch is compatible, and should not cause any breaking change. Curiously, lib passed but es failed, is it cause by this? https://github.com/ant-design/ant-design/blob/62233426f30b2b7508291a15583aa9f251df0622/.github/workflows/test.yml#L364-L370

@zombieJ
Copy link
Member

zombieJ commented Apr 12, 2022

Find the reason. Origin getContainer is called in Portal render. But current logic makes render div on the Trigger render which raise the render before. Let me patch this.

zombieJ added a commit that referenced this pull request Apr 12, 2022
@tmkx
Copy link
Contributor Author

tmkx commented Apr 12, 2022

Find the reason. Origin getContainer is called in Portal render. But current logic makes render div on the Trigger render which raise the render before. Let me patch this.

Got it, it should like this:

<Portal
  getContainer={() => {
    if (xxx) {
      // ...
    }
    return ...
  }}
  ...
  />

zombieJ added a commit that referenced this pull request Apr 12, 2022
* Revert "Revert "fix: prevent from creating multi portals (#292)" (#293)"

This reverts commit fd694b6.

* chore: patch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Portal is not working properly in StrictMode
2 participants