-
-
Notifications
You must be signed in to change notification settings - Fork 319
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(virtual-core): add support to multi window #738
Conversation
@@ -79,7 +83,7 @@ export const observeElementRect = <T extends Element>( | |||
return () => {} | |||
} | |||
|
|||
const observer = new ResizeObserver((entries) => { | |||
const observer = new targetWindow.ResizeObserver((entries) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also check if ResizeObserver is on targetWindow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it should be guarded by type as well. (If you meant by checking if (targetWindow.ResizeObserver)
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, same as in comment below, we check if ResizeObserver is on global context on L82, but the create instance on targetWindow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated :) could you give it one more round of review
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 8a60b26. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
Thanks @YuanboXue-Amber great work 👏 |
@piecyk thanks for the quick review! When do you plan the next release? We would like to use have this change in our app. |
Overview
In multi window environment, where React is running in a main window/process and renders content to a different child window, the globals like
setTimeout
/clearTimeout
will not work properly because it's using the main Window object and not the child one.This PR adds support to multi-window.
setTimeout
-->targetWindow.setTimeout
clearTimeout
-->targetWindow.clearTimeout
ResizeObserver
-->targetWindow.ResizeObserver
targetWindow
is obtained fromscrollElement.ownerDocument.defaultView
.More details
We have multi-windows use case, so we want to make sure that the globals work well with it. This is the reason for my PR.
I built an example of dynamic height list in child window:
https://stackblitz.com/edit/vitejs-vite-2zau9t?file=src%2FApp.tsx&terminal=dev
(Note that this happens only when the child window is opened for the first time from the main window. This inconsistent behavior came from browser.)
I tried the same example with my change, and the resize is triggered immediately.
before-480p.mov
after-480p.mov