Experimental VirtualList Optimization

[azizhk]

This is VirtualList Optimization done by @christianbach in #21163 enabled behind a prop: experimentalVirtualizedListOpt with backward compatibility.

Summary

Fixes #20174 based of @jasekiw pull request #20208

Changelog

// TODO:

[CATEGORY] [TYPE] - Message

Test Plan

// TODO:
Add tests related to backward compatibility. (Will need help)

[pull-bot]

Messages

📖

📋 Verify Changelog Format - A changelog entry has the following format: [CATEGORY] [TYPE] - Message. CATEGORY may be: General iOS Android JavaScript Internal (for changes that do not need to be called out in the release notes) TYPE may be: Added, for new features. Changed, for changes in existing functionality. Deprecated, for soon-to-be removed features. Removed, for now removed features. Fixed, for any bug fixes. Security, in case of vulnerabilities. MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against 5e9a311

[christianbach]

@cpojer this seems like a more reasonable approach. What can we do to move this along?

[dominiczaq]

Is there a way to help get this merged?

[abmantis]

This is open for 9 months already. Any way to get some attention to this?

[ksitko]

Is there any reason why this has been left open?

[lunaleaps]

Some thoughts about how we do experimentation with this.

I'm hesitant about this approach because we're wrapping a Context around everyone's sticky header.
I'm thinking we approach this like the component re-factors we've been doing where we create an injection that is up to product teams to include in their bundle. The injection is read then instead of a new prop experimentalVirtualizedListOpt.

See how we're doing Animated refactor:

• Injection: https://github.com/facebook/react-native/blob/1465c8f3874cdee8c325ab4a4916fda0b3e43bdb/Libraries/Animated/createAnimatedComponentInjection.js
  Then in our Bundle initialization we have:

import createAnimatedComponentInjection from 'createAnimatedComponentInjection';
import createAnimatedComponent_EXPERIMENTAL from 'createAnimatedComponent_EXPERIMENTAL';
...

  if (<our experiment framework says you're in this test>) {
    createAnimatedComponentInjection.inject(
      createAnimatedComponent_EXPERIMENTAL,
    );
  }

So for this case we'd probably read the injection value like so:

    if (OUR_INJECTION.useExperimentalThing) {
      child.props.onLayout(event, child.props.cellKey, child.props.index);
    } else {
      child.props.onLayout(event);
    }

What do you think about this approach? I'm happy to import and make these changes

[azizhk]

What do you think about this approach? I'm happy to import and make these changes

I didn't quite get you, but am happy to go through your PR.

[facebook-github-bot]

@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[lunaleaps]

@azizhk What kind of tests do you need help writing for this?

[lunaleaps]

I believe this would be a new prop we're passing through in the "non-experiment" path. Can we update this to ensure we're not changing original behavior?

[lunaleaps]

@azizhk Here is what I meant by using an injection: https://github.com/facebook/react-native/pull/31947/files

Somewhere in your bundle initialization, you can enable the experiment by

import {setUseVLOptimization} from 'react-native/Libraries/Lists/VirtualizedListInjection';
...
if (yourExperimentFramework.isInExperiment) {
   setUseVLOptimization();
}

[azizhk]

@azizhk What kind of tests do you need help writing for this?

Basically I need to add tests which ensure that we are not re-rendering the rows. So something like re-render the virtual list, don't expect the cells to be rendered again.

[lunaleaps]

@azizhk's comment on https://github.com/facebook/react-native/pull/31947/files (a demonstration PR)

Ah, I see. I love this approach. 😍 Less Nesting, Less Props.
Just that now in the same tree, you can't have two versions of VirtualList one with optimization and one without. (Which I am totally fine with)

Mm that's a good point. I think since this is a question of optimizing overall that's a fine tradeoff. We'll want to run an experiment internally.. to be honest I'm not sure I understand the optimization... might have to scan the issue history a bit more closely or if you could give me TL;DR?

[github-actions]

This PR is waiting for author's feedback since 24 days. Please provide the requested feedback or this will be closed in 7 days

[github-actions]

This PR was closed because the author hasn't provided the requested feedback after 7 days.