Fix for #36828 - props not being copied into constructor

[Kyle772]

Summary:

Fixes an error when trying to render FlatLists where the local class variable this.props is undefined preventing the component from getting to first render.

This is the change that introduced super(props) ad733ad

For some reason super(props) is not lifting the props up into the parent class constructor. I think this could be due to the changes in this commit which added a wrapper component and removed the legacy implementation. (I have no idea what this is and didn't look further though I suspect that the legacy implementation handled the passing of the props previously and the virtualized one does not for some reason.

af4903e

I ran into this issue after updating my RN expo project from SDK 49 to SDK 50 which bumps react native up to 0.73 from 0.72.4 to give a possible trail on where and how this broke on this type of setup.

Changelog:

[GENERAL] [FIXED] - added a single line
this.props = props which takes the passed props and stores them in the class constructor
this change likely is not solving the actual problem here but it does skirt the issue where the error is being raised. I think this may need a deeper look by the RN team.

Test Plan:

Quite a few people mentioned this in the comments of issue #36828 and other similar threads. All signs point to the issue being with the flatlist file. NickGerleman originally came up with the solution and a handful of people have made this change with a patch package with success but none submitted a PR. I can verify that this works in the interim until another solution is found.

[facebook-github-bot]

Hi @Kyle772!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[NickGerleman]

@Kyle772 could you please share your Babel config?

[Kyle772]

@Kyle772 could you please share your Babel config?

module.exports = function (api) {
    api.cache(true);
    return {
        presets: ['babel-preset-expo'],
        plugins: [
            ["@babel/plugin-transform-flow-strip-types", { loose: true }],
            ["@babel/plugin-proposal-class-properties", { loose: true }],
            ["@babel/plugin-proposal-private-methods", { loose: true }],
            ['module:react-native-dotenv', {
                'moduleName': '@env',
                'path': '.env',
                'blacklist': null,
                'whitelist': null,
                'safe': false,
                'allowUndefined': true
            }],
            ['module-resolver', {
                alias: {
                    'components': './components',
                    'api': './api',
                    'utils': './utils',
                    'types': './types',
                    'hooks': './hooks',
                    'constants': './constants',
                    'assets': './assets',
                    'contexts': './contexts',
                    'wrappers': './wrappers'
                }
            }],
        ],
    };
};

Throughout this process I've audited my whole babel config and all of the entries are needed for what I'm working with. Some people mentioned removing private-methods or adding strip types I ended up already having and needing both for a few crypto libs I'm using. In any case, I did try removing it regardless and it didn't solve the error I was getting.

[NickGerleman]

Does your project specify a version of babel-preset-expo. If so, what version?

Some people mentioned removing private-methods or adding strip types I ended up already having and needing both for a few crypto libs I'm using. In any case, I did try removing it regardless and it didn't solve the error I was getting.

Could you try removing them from package.json and forcing install? I have a hunch that some babel plugins, versions, configs, might not be lining up.

A step to debug this could be to look at what the transformed result is. E.g. ask Metro server for the FlatList module and look at the result.

Also FYI @robhogan in case you have any hunches why we might keep on seeing code issues, specifically in FlatList, alongside some specific babel config patterns.

[Kyle772]

Does your project specify a version of babel-preset-expo. If so, what version?

"@babel/core": "^7.20.0", -> 7.23.2
"babel-preset-expo": "^10.0.1", -> 10.0.1

Could you try removing them from package.json and forcing install? I have a hunch that some babel plugins, versions, configs, might not be lining up.

So I went ahead and removed all of my babel plugins just to get this tested for you, opening up a flat list provides the following now.

In the FlatList.js line referenced there is this

Which is just the props being undefined as you previously mentioned. Looking through the code further I'm noticing it's actually the this._checkProps that is breaking here and I think it is due to it checking this.props instead of the props being passed into the constructor. Race condition? not sure but I'm going to push up the change fixing that check props call.

this._checkProps(this.props)
to
this.checkProps(props)

Is this.props available to the constructor if the constructor is being called before the component has mounted? this._checkProps is being called in componentDidUpdate and that one doesn't seem to be firing any errors.

[Kyle772]

Reproducing this error is very hit or miss.

this._checkProps(props); always seems to work
this._checkProps(this.props); seems to break every other reload

[Kyle772]

Looking at VirtualizedList.js which is what FlatList is calling it also uses this pattern. I suspect the bug was fixed on one instead of both of these files.

Initially added here in 73.4 which is right at the cutoff for expo 49
aab9df3

[dieguezz]

This has already been fixed here e33767a #43141 (review)

[dieguezz]

@Kyle772 maybe you can also close #34783 and #42623

[Kyle772]

@Kyle772 maybe you can also close #34783 and #42623

@NickGerleman