Skip to content

fix(virtual-core): add support to multi window - #738

Merged
piecyk merged 4 commits into
TanStack:mainfrom
YuanboXue-Amber:gobal
Jun 5, 2024
Merged

fix(virtual-core): add support to multi window#738
piecyk merged 4 commits into
TanStack:mainfrom
YuanboXue-Amber:gobal

Conversation

@YuanboXue-Amber

Copy link
Copy Markdown
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!

Comment thread packages/virtual-core/src/index.ts Outdated
Comment thread packages/virtual-core/src/index.ts Outdated
Comment thread packages/virtual-core/src/index.ts Outdated
Comment thread packages/virtual-core/src/index.ts Outdated
Comment thread packages/virtual-core/src/index.ts Outdated
}

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@nx-cloud

nx-cloud Bot commented Jun 4, 2024

Copy link
Copy Markdown

☁️ 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
@piecyk

piecyk commented Jun 5, 2024

Copy link
Copy Markdown
Collaborator

Thanks @YuanboXue-Amber great work 👏

@YuanboXue-Amber

Copy link
Copy Markdown
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

piecyk commented Jun 5, 2024

Copy link
Copy Markdown
Collaborator

@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