React-native: Removing TimerMixin, SubscribableMixin from components.

[TheSavior]

As part of our goal to modernize and type our core components, we need to migrate off of mixins. This task is to track the work to remove TimerMixin from our codebase.

This is step 2. Step 1 can be found here.

TimerMixin enables components to call functions like this.setTimeout and have that timeout automatically cleared when the component is unmounted. In order to remove TimerMixin from these components, we will need to do this work manually.

The following are the individual files that currently use TimerMixin which will need to be migrated. Each file should be migrated in its own PR and each file is about the size of a good first issue to learn and get familiar with a part of the React Native codebase. If you’d like to convert one, pick one from the list, comment on this issue that you are interested in converting it, and follow the tips below for help successfully updating that file.

• [x] IntegrationTests/ReactContentSizeUpdateTest.js
• [x] IntegrationTests/TimersTest.js
• [x] Libraries/Components/TextInput/TextInput.js
• [x] Libraries/Components/Touchable/TouchableOpacity.js
• [x] Libraries/Components/Touchable/TouchableWithoutFeedback.js
• [x] Libraries/Experimental/SwipeableRow/SwipeableRow.js
• [x] Libraries/Lists/ListView/ListView.js (in progress)
• [x] RNTester/js/ProgressBarAndroidExample.android.js
• [x] RNTester/js/ProgressViewIOSExample.js
• [x] RNTester/js/TimerExample.js

Here is an example component that uses setTimeout from TimerMixin.

createReactClass({
  mixins: [TimerMixin],

  render() {
    return (
      <View
        onPress={() => {
          this.setTimeout(() => {
            alert('hi');
          });
        }}>
        Whee
      </View>
    );
  },
});

This would need to be changed to:

createReactClass({
  _timeoutID: (null: ?TimeoutID),

  componentWillUnmount() {
    if (this._timeoutID != null) {
      clearTimeout(this._timeoutID);
    }
  },

  render() {
    return (
      <View
        onPress={() => {
          this._timeoutID = setTimeout(() => {
            alert('hi');
          });
        }}>
        Whee
      </View>
    );
  },
});

For a more complete example you can look at this commit which removes TimerMixin from TimerExample in RNTester.

When submitting a PR removing an instance of TimerMixin, it is important to include a good test plan. This test plan helps reviewers understand the work you did to verify that your changes are correct and complete. Since this changes the runtime logic of these components, it is important that these changes are tested in an app. For most of these components, you should be able to validate your change via RNTester. If there is nothing that exists as-is in RNTester which gives you confidence in your change, adding something to RNTester would be greatly appreciated. :)

[jerodestapa]

I'll take IntegrationTests/TimersTest.js.

[exced]

Hey, I'll take Libraries/Lists/ListView/ListView.js

[danibonilha]

Hi! I'll take Libraries/Components/Touchable/TouchableOpacity.js = )

[Thomas0c]

I'll grab Libraries/Components/Touchable/TouchableWithoutFeedback.js.

[gvarandas]

I'll take Libraries/Experimental/SwipeableRow/SwipeableRow.js 🤖

[richardcann]

Hi! I'll take Libraries/Components/TextInput/TextInput.js

[nd-02110114]

Hi!
I'll take RNTester/js/ProgressBarAndroidExample.android.js

[peaonunes]

Hello, I will take IntegrationTests/ReactContentSizeUpdateTest.js :)

[gvarandas]

I'll get RNTester/js/ProgressViewIOSExample.js 🤖

[ronal2do]

Are they done? Is there none left for me?    😭😭😭

[sopranolinist]

Missed this one due to life. @TheSavior please keep me in the loop for future contributions on this project, thanks!

[TheSavior]

You all are amazing! Thanks for going through these.

@ronal2do and @sopranolinist, There will be plenty more to clean up and migrate. I just can't post directions fast enough to keep up with all of these contributions. :)

We've merged all the PRs you've sent us and it looks like there is only one more with TimerMixin. @jerodestapa was interested in doing it a few days ago but since we haven't received a PR yet, someone else can probably send that one. There will be more if @jerodestapa wants to do some other changes in the future.

Next Step:

There are three components using Subscribable.Mixin, I'd appreciate PRs for those. Subscribable.Mixin is essentially the same as TimerMixin. It lets you register an event listener, and cleans it up in componentWillUnmount. Lets just inline those checks like we did with TimerMixin so that we can delete Subscribable.Mixin. Here is an example commit converting one of the three.

• [x] ReactContentSizeUpdateTest
• [x] SizeFlexibilityUpdateTest
• [x] ScrollResponder

[nd-02110114]

Hi! thank you for next step!!
I will take ScrollResponder.

[exced]

I'll take SizeFlexibilityUpdateTest

[exced]

@TheSavior Do we get rid of createReactClass by the way ?

[TheSavior]

@exced, if you can focus this PR on just the SubscribableMixin that would make it easier to review. I'm going to post something separate about removing createReactClass.

[TheSavior]

Since I can't keep up as much as you all seem to want, I put up another issue for converting to createReactClass with a bit longer of a list of files. :) That is one of the final steps so the list isn't complete. As we finish removing mixins in this issue and others, more files will be able to get added to that list.

Thanks for all your help so far!

[jerodestapa]

Sorry, @TheSavior! I got busy with some things and couldn't get back to it. I will definitely jump back in when I have more time, though.

[TheSavior]

Thanks for all the work everyone! Looks like the only thing left for this issue is removing TimerMixin from IntegrationTests/TimersTest.js. Can someone send a diff for that one? Then we can close this out :)

[danibonilha]

I can take that one! (TimersTest)

[danibonilha]

@TheSavior I've just started, but I'm seeing it's a big one, if you guys are in a hurry let me know that I will pass it forward because it's release week at work and I don't have a lot of free time, if not, I can take it on the weekend.

[danibonilha]

I'm sorry, I'm working overtime =/. So IntegrationTests/TimersTest.js is available again if anyone wants to grab it.

[exced]

No worries :)
I can take IntegrationTests/TimersTest.js

[RSNara]

@exced and @ronal2do, It looks like there was a conflict for IntegrationTests/TimersTest.js, and I independently reviewed both without realizing they were doing the same thing. 😅

Before I noticed, I imported #21748 internally and made a few modifications to it. @exced, I hope it's okay if I close #21772 and land #21748 instead, since #21748 is already further along in the fb-internal landing process. Thank you to both of you for launching the PRs! But in the future, we should be more careful about these conflicts. 😔

[TheSavior]

You are all amazing. Thanks for all the contributions. This issue is done!