Ensure that "scrollToIndex" function continues to work when errors occur

[m-a-r-c-e-l-i-n-o]

Hey @inokawa, so this PR is more symbolic than anything else because I think you can come up with a better way to go about this. It has to do with a side effect of the changes that were made related to #733

The good news is that with the changes you've made so far (version: 0.43.4), I have not been able to reproduce any of the browser freezing that I spoke about before, however under the same conditions, I now get an undefined "uncaught" error that prevents the scrollToIndex() from working when initially loading the list. I have tracked that down to the activity in scheduleImperativeScroll. More specifically the await initialized; that errors out and prevents the viewportElement!.scrollTo from running. Seems that when the error occurs, the viewportElement element is still there and calling viewportElement!.scrollTo() is what makes the list work as expected when initially loading, despite the error.

The changes I am suggesting in this PR allows the code to continue down to execute viewportElement!.scrollTo, but tries to prevent the while (true) from getting out of control, which I theorize that it (the while loops) must be what was causing the browser freezing before. I plan on going to back to previous versions of Virtua to double check, but it would make sense that it would be caused by something running non-stop forever, hugging up all of the resources and freezing everything. Which begs the question, why use while loops with promises? Maybe something more insightful like a recursive function might a better measure, considering that a stack overflow error might make it easier to pin point the general nature of the freeze.

At any rate, the changes proposed here passes all of the tests (and works as expected on my app), but it brings about multiple errors:

The "uncaught" error is understandable because it's being thrown by the cancelScroll, which we can just suppress. But I have no idea what the SES_UNCAUGHT_EXCEPTION error is truly about.

The SES_UNCAUGHT_EXCEPTION error:

I suppose that another possibility for the freezing would have been microtask because of:

But that is far more relevant to #733 than to this PR.

[inokawa]

@m-a-r-c-e-l-i-n-o
Hi, what do you think if it just returns without reject when it errors?

    const ok = await initialized;
    if (!ok) {
      return 
    }

The uncaught reject means the Virtualizer component was unmounted. So I think we shouldn't continue the current scrollToIndex and should reschedule again on next remount.

The freezing was probably caused by microtask() call, because microtask is usually scheduled earlier than animation frame.

[m-a-r-c-e-l-i-n-o]

@m-a-r-c-e-l-i-n-o Hi, what do you think if it just returns without reject when it errors?

    const ok = await initialized;
    if (!ok) {
      return 
    }

The uncaught reject means the Virtualizer component was unmounted. So I think we shouldn't continue the current scrollToIndex and should reschedule again on next remount.

The freezing was probably caused by microtask() call, because microtask is usually scheduled earlier than animation frame.

I agree with you that in theory we shouldn't continue the current scrollToIndex and should reschedule again on next remount but if we do it the way you are suggesting, the initial scrollToIndex() calls simply don't work on mount and so our chat doesn't scroll all the way to bottom as it should. At the bare minimum we need:

Maybe there is some state that is bleeding because somehow the viewportElement persists through all this and needs the scroll offset to be set even when it's erroring out in order for scrollToIndex() to work as expected on mounting. I wish I knew more of why this is, because I agree with you that it technically doesn't make sense.

Lastly, your suggestion would get rid of the "uncaught" error, but the SES_UNCAUGHT_EXCEPTION remains, although this error does not seem to have an impact on functionality, so not all that relevant.

[inokawa]

Thank you. And I'd like to know well how you are using this component.

If the lifecycle of Virtualizer and the scrollable element is the same, it's ok to cancel scrollTo and reschedule on remount, as I suggested.

const List = () => {
  const ref = useRef(null);
  const scrollRef = useRef(null);
  useEffect(() => { ref.curernt.scrollToIndex(...) }, [])
  return (
    <div style={{ overflowY: "auto" }} ref={scrollRef}>
      <Virtualizer ref={ref} scrollRef={scrollRef}>
        {...}
      </Virtualizer>
    </div>
  );
}

But if it's not the same, if virtualizer unmounts but the parent does not, maybe we should not cancel scrollTo. For example, in the case below, React may not unmount viewportElement even if mounted is false:

const List = () => {
  const ref = useRef(null);
  const scrollRef = useRef(null);
  return mounted ? (
    <div style={{ overflowY: "auto" }} ref={scrollRef}>
      <Virtualizer ref={ref} scrollRef={scrollRef}>
        {...}
      </Virtualizer>
    </div>
  ) : (
    <div style={{ overflowY: "auto" }} ref={scrollRef}>
      <SomeComponent />
    </div>
  );
};

[m-a-r-c-e-l-i-n-o]

@inokawa I am not using Virtualizer directly, I'm using the VList component with the reverse option, and looking at the source code for VList, I'd say that it's very similar to the second scenario if virtualizer unmounts but the parent does not, which makes sense because the reverse option introduces the external scrollRef inside VList. Here's what our component currently looks like, and the issue seems to be fundamentally tied to the ridiculously quick mount/unmount (just a few milliseconds in between), which you can see on the console messages on the second screenshot below.

[inokawa]

I removed all promise rejections(including #750 (comment)) from virtua in 0.43.5. So at least unhandled promise rejection will not happen anymore.

I'm not sure if the problem still exists or not. Also not sure what SES_UNCAUGHT_EXCEPTION is (probably injected script by browser extention or some frameworks?), but perhaps not related as you mentioned.

[m-a-r-c-e-l-i-n-o]

Hey @inokawa, the errors are gone, but the issue with scrollToIndex persists because ! (await initialized) gets stuck on true which causes scheduleImperativeScroll to return early and thus never gets to execute viewportElement!.scrollTo. The good news is that I was able to isolate it. Please pull the latest from https://github.com/m-a-r-c-e-l-i-n-o/virtua-debug and have a closer look at what I mean.

You should be able to see that the chat doesn't scroll down on page reload about 90% of the time and about 50% of the time it will still be broken after 3 seconds. Usually when it's broken after 3 seconds, it's permanently broken — the scrollToIndex simply never recovered and is useless from that point forth despite the everything else running fine.