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

Just-in-time measured content [work in progress] #102

Closed
wants to merge 50 commits into from
Closed

Conversation

bvaughn
Copy link
Owner

@bvaughn bvaughn commented Dec 7, 2018

Initial implementation of a list component supporting just-in-time measured content (relates to issue #6).

Checklist

  • Gracefully degrade when ResizeObserver API is not available.
  • Fallback to using findDOMNode when no DOM element ref is available, but warn about this.
  • Ignore resize events when hidden (see this comment)
  • Use RAF to batch multiple updates (for MO polyfill, which is async?)
  • RTL support
  • Negative offsets check to fix Safari's elastic scroll issue (Fix elastic bounce issues and RTL issues #251)
  • Smoke test the following browsers/platforms:
    • Desktop
      • Chrome
      • Edge
      • Firefox (momentum scrolling caveat for Firefox <= 64, fe88aeb)
      • Internet Explorer
      • Safari
    • Mobile
      • Android
      • iOS (momentum scrolling jumps around)

Brian Vaughn added 30 commits June 17, 2018 10:03
Merging master into issues/6 branch
@natew
Copy link

natew commented Apr 2, 2019

@piecyk thank you!

@bvaughn noticed the version is bumped to 1.8.0-alpha but not published, could you publish when you get a chance?

If the resulting ref was null, the old reference wasn't getting reset, and we'd end up re-observing the original node.
@edgars-sirokovs
Copy link

@piecyk thank you!

@bvaughn noticed the version is bumped to 1.8.0-alpha but not published, could you publish when you get a chance?

Hi! Any chance this will be published in the near future?

@zmitry
Copy link

zmitry commented May 18, 2019

Any updates on this ?

edgars-sirokovs and others added 4 commits May 21, 2019 14:12
Fix item data not passed to DynamicSizeList itemKey
Fixed a memory leak in `ItemMeasurer`
Fix item data not passed to itemKey
@Hellycat
Copy link

May you inform about approximate date of merging this PR into master branch?

@natew
Copy link

natew commented Jun 22, 2019

I've found a behavior we may want to avoid.

Basically, I have a virtual list inside a div that will be set to style={{ display: none }} when hidden. This triggers the ResizeObserver to update, which causes many extra renders, as well as visual flickering because react-window thinks the list height has gone to 0.

We could make ItemMeasurer customizable as a prop, so I could customize the resize observing logic.

@Bessonov
Copy link

@natew what is the goal to set display: none? For me, it's expected and valid behavior.

@natew
Copy link

natew commented Jun 22, 2019

To prevent resizing the window from triggering paints within the entire parent container that includes that list.

I don’t think you can claim it’s expected given you don’t have the use case.

Re rendering on display none doesn’t make sense in this case. Further, if you display it again you see a flicker where the list items are all overlapped at top = 0 while it measures and then shows again. So it’s one bug and one extra render.

One way to think of it being “expected” is to ask does it behave as similarly to a non-virtual list as possible. A non virtual list wouldn’t re render on display changes.

I submitted a PR that let’s you override CellMeasurer. It’s not perfect, the bug still will happen without using a custom one, but it works for this case and maybe others.

@bvaughn
Copy link
Owner Author

bvaughn commented Jun 22, 2019

The built-in measuring solution needs to handle hidden/display:none as per the TODO item in the PR description:

Ignore resize events when hidden

I don't think requiring an overridden measurer is an approach I want to take. I'd rather this just work out of the box. I have a rough idea for how we might tackle it.

@natew
Copy link

natew commented Jun 22, 2019 via email

@john-osullivan
Copy link

@natew I just ran into an issue like yours because my list lives in an @reach/tabs Tab and the implementation sets non-visible tabs to display:none. My list is pretty fat (1k min), so I was getting a full React crash when it was hidden (Uncaught Invariant Violation: Maximum update depth exceeded.).

It's not an ideal solution, but given that we're writing "in-the-meantime" hacks anyways, I found that using tab visibility to switch from a DynamicSizeList to a FixedSizeList is performant enough and gets around this temporary issue. You do lose your scroll position as was mentioned in the comment linked on the todo, though, so definitely not a long-term answer.

I know this is just a side project for you @bvaughn ; if you can explain your rough thinking on how you'd like to approach this, I'd be happy to lend a hand with writing up the final solution 👍

@renzosal
Copy link

Hi! any chance 1.9.0-alpha.1 could be published?

@brunolemos
Copy link

brunolemos commented Jun 29, 2019

@bvaughn: found a bug in this branch: #278

@john-osullivan
Copy link

john-osullivan commented Jul 10, 2019

Hallo to fellow react-window users eager for this feature to be released. I ran into an issue with my CI/CD environment not properly building packages installed from git, so my prior workaround doesn't work for production deployment. In order to solve it, I forked and published the contents of this branch @ bc9192b as a scoped package:

npm i @john-osullivan/react-window-dynamic-fork

In order to prevent myself from forgetting to update when the feature is released, I added a file which runs at buildtime and checks the most recent published version of react-window. You can see it on branch issues/6 of my fork. If the version satisfies >=1.9.0, it throws an error and exits the process telling you to uninstall my fork and put the proper package back in:

const npmApi = require('npm-api');
const semver = require('semver');
const process = require('process');
const npm = new npmApi();
const reactWindow = npm.repo('react-window');

reactWindow.package().then((pkgJson) => {
  let version = pkgJson.version;
  if (semver.satisfies(semver.coerce(version), '>=1.9.0')) {
    throw new Error(`react-window ${version} has been released, please uninstall this fork and reinstall react-window.`);
  } else {
    console.log(`\nMost recent react-window version is ${version}, DynamicList not yet available on npm.`);
    console.log('This fork package will inform you when react-window @ 1.9.0 is available. \n')
  }
}).catch((err) => {
  console.log(`\n${err}\n`);
  process.exit(1)
});

Obviously use at your own risk, as this PR isn't complete. Happy hacking! @renzosal @tony-scio @patryksuchowierski @Hellycat @jazevedo620 @satyadeepk

Edit: Bringing over the sample code (Autosized list with FunctionComponent items) from my earlier comment so I only have one wall of text on this PR. Note the usage of React.forwardRef()! If you haven't had to deal with them before, refs let React connect to the DOM -- if you don't attach refs to your elements, then there's no way for them to be measured.

      <AutoSizer>
        {({ height, width }) => (
          <DynamicSizeList itemCount={items.length}
            itemData={items}
            itemKey={index => items[index].itemKey}
            height={height} 
            width={width}>
            {
              React.forwardRef(({ data, index, style }, ref) => (
                  <ItemComponent
                    ref={ref}
                    itemData={data[index]}
                    style={style}  />
              ))
            }
          </DynamicSizeList>
        )}
      </AutoSizer>

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.