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(virtual-core): add support to multi window #738

Merged
merged 4 commits into from
Jun 5, 2024

Conversation

YuanboXue-Amber
Copy link
Contributor

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 from scrollElement.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

  1. Open this example on Chrome/Edge
  2. Popout the example in a new tab
  3. Click 'Open Child Window' button, a child window opens and a dynamic height virtual list is visible
  4. Open devtools on child window, modify the content of Row 0.
  5. Notice the resize did not get triggered immediately. It triggers after 5-10 seconds.
    (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
before-480p.mov
  • After
after-480p.mov

We are switching to @tanstack/react-virtual for our virtualized lists because we find its api very appealing!
This is my first time contributing to tanstack. I appreciate your feedbacks!

packages/virtual-core/src/index.ts Outdated Show resolved Hide resolved
packages/virtual-core/src/index.ts Outdated Show resolved Hide resolved
packages/virtual-core/src/index.ts Outdated Show resolved Hide resolved
packages/virtual-core/src/index.ts Outdated Show resolved Hide resolved
packages/virtual-core/src/index.ts Outdated Show resolved Hide resolved
@@ -79,7 +83,7 @@ export const observeElementRect = <T extends Element>(
return () => {}
}

const observer = new ResizeObserver((entries) => {
const observer = new targetWindow.ResizeObserver((entries) => {
Copy link
Collaborator

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

Copy link
Contributor Author

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))

Copy link
Collaborator

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

Copy link
Contributor Author

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

Copy link

nx-cloud bot commented Jun 4, 2024

☁️ Nx Cloud Report

CI 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 target

Sent with 💌 from NxCloud.

@piecyk piecyk merged commit 19b4272 into TanStack:main Jun 5, 2024
2 checks passed
@piecyk
Copy link
Collaborator

piecyk commented Jun 5, 2024

Thanks @YuanboXue-Amber great work 👏

@YuanboXue-Amber
Copy link
Contributor Author

@piecyk thanks for the quick review! When do you plan the next release? We would like to use have this change in our app.

@YuanboXue-Amber YuanboXue-Amber deleted the gobal branch June 5, 2024 11:36
@piecyk
Copy link
Collaborator

piecyk commented Jun 5, 2024

@YuanboXue-Amber it alredy done https://github.com/TanStack/virtual/releases/tag/v3.5.1

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.

2 participants