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

Removing TimerMixin, SubscribableMixin from components. #21485

Closed
10 tasks done
elicwhite opened this issue Oct 4, 2018 · 25 comments
Closed
10 tasks done

Removing TimerMixin, SubscribableMixin from components. #21485

elicwhite opened this issue Oct 4, 2018 · 25 comments
Labels
Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Help Wanted :octocat: Issues ideal for external contributors. JavaScript Resolution: Locked This issue was locked by the bot.

Comments

@elicwhite
Copy link
Member

elicwhite commented Oct 4, 2018

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.

  • IntegrationTests/ReactContentSizeUpdateTest.js
  • IntegrationTests/TimersTest.js
  • Libraries/Components/TextInput/TextInput.js
  • Libraries/Components/Touchable/TouchableOpacity.js
  • Libraries/Components/Touchable/TouchableWithoutFeedback.js
  • Libraries/Experimental/SwipeableRow/SwipeableRow.js
  • Libraries/Lists/ListView/ListView.js (in progress)
  • RNTester/js/ProgressBarAndroidExample.android.js
  • RNTester/js/ProgressViewIOSExample.js
  • 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. :)

@elicwhite elicwhite added Good first issue Interested in collaborating? Take a stab at fixing one of these issues. JavaScript Help Wanted :octocat: Issues ideal for external contributors. labels Oct 4, 2018
@jerod-estapa
Copy link

I'll take IntegrationTests/TimersTest.js.

@exced
Copy link
Contributor

exced commented Oct 4, 2018

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

@danibonilha
Copy link
Contributor

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

@Thomas0c
Copy link
Contributor

Thomas0c commented Oct 4, 2018

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

@gvarandas
Copy link
Contributor

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

@richardcann
Copy link
Contributor

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

@nissy-dev
Copy link
Contributor

nissy-dev commented Oct 5, 2018

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

@peaonunes
Copy link
Contributor

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

@gvarandas
Copy link
Contributor

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

@ronal2do
Copy link
Contributor

ronal2do commented Oct 5, 2018

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

facebook-github-bot pushed a commit that referenced this issue Oct 5, 2018
Summary:
Related to #21485.
Removed TimerMixin from the `RNTester/js/ProgressBarAndroidExample.android.js` since it is currently not used.

- [x] npm run prettier
- [x] npm run flow-check-ios
- [x] npm run flow-check-android

In progress 🙇

[GENERAL] [ENHANCEMENT] [RNTester/js/ProgressBarAndroidExample.android.js] - remove TimerMixin dependency
Pull Request resolved: #21501

Reviewed By: TheSavior

Differential Revision: D10218375

Pulled By: RSNara

fbshipit-source-id: c4c12f65855452bc2485f034a0560afd204512f4
facebook-github-bot pushed a commit that referenced this issue Oct 5, 2018
Summary:
Related to #21485.
Removed TimerMixin from the `RNTester/js/ProgressViewIOSExample.js` screen since it is currently not used.

- [x] `npm run prettier`
- [x] `npm run flow-check-ios`
- [x] `npm run flow-check-android`
- [x] runtime tests using `ProgressViewIOSExample` on Android and iOS

**RNTester steps**

- [x] Run RNTester.
- [x] Navigate to `ProgressViewIOSExample` and check if the animations are executed correctly and without lag.

[GENERAL] [ENHANCEMENT] [RNTester/js/ProgressViewIOSExample.js] - remove TimerMixin dependency
Pull Request resolved: #21500

Reviewed By: TheSavior

Differential Revision: D10218366

Pulled By: RSNara

fbshipit-source-id: b44a0bbb50f6b0e85f406904131804eace941335
facebook-github-bot pushed a commit that referenced this issue Oct 5, 2018
Summary:
Related to #21485

- Remove TimerMixin from ListView

- All flow tests succeed.
- RNTester: <ListView> iOS (this change should only affect iOS because calculateChildFrames is iOS only)
Show perf monitor, show ListView* screen, start scrolling. UI frame Rate is used at the beginning. When scrolling there is no drop in FPS rate.

TODO: I think a load test would be more relevant:
- Update props multiple times and scroll

[GENERAL] [ENHANCEMENT] [ListView.js] - rm TimerMixin
Pull Request resolved: #21488

Differential Revision: D10219088

Pulled By: RSNara

fbshipit-source-id: 946e4fc1319324c5bf4947a2060b18bebb6fc493
facebook-github-bot pushed a commit that referenced this issue Oct 5, 2018
Summary:
Related to #21485.
Removed `TimerMixin` from the `TouchableWithoutFeedback` component since it is currently not used.
Added tests cases for `TouchableWithoutFeedback` to check for any runtime issues.
Pull Request resolved: #21493

Differential Revision: D10219098

Pulled By: RSNara

fbshipit-source-id: d9517b2bd5b72b0450fa864f3556673ae3181552
facebook-github-bot pushed a commit that referenced this issue Oct 5, 2018
Summary:
Related to #21485.
Removed `TimerMixin` from `ReactContentSizeUpdateTest`.
Pull Request resolved: #21502

Reviewed By: TheSavior

Differential Revision: D10218548

Pulled By: RSNara

fbshipit-source-id: 9a0642e14f8548d8dc9683f0f70353416ed04ae0
@sopranolinist
Copy link

sopranolinist commented Oct 5, 2018

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

facebook-github-bot pushed a commit that referenced this issue Oct 6, 2018
Summary:
Related to #21485
This PR removes TimerMixin from TouchableOpacity
Pull Request resolved: #21520

Differential Revision: D10223753

Pulled By: RSNara

fbshipit-source-id: fc02077de7e73ee968b7944c0178892825099063
facebook-github-bot pushed a commit that referenced this issue Oct 8, 2018
Summary:
Related to #21485.
Removed TimerMixin from the SwipeableRow component since it is currently not used.
Added a test case for `SwipeableRow` animation in the `SwipeableListViewSimpleExample`, by adding the `bounceFirstRowOnMount` prop, to check for any runtime issues with the setTimeout method.

- [x] `npm run prettier`
- [x] `npm run flow-check-ios`
- [x] `npm run flow-check-android`
- [x] runtime tests using `SwipeableFlatListExample` on Android and iOS
- [x] runtime tests using `SwipeableListViewSimpleExample` on Android and iOS

**RNTester steps**

- [x] Run RNTester.
- [x] Navigate to `SwipeableFlatListExample` and check if the `_animateBounceBack` animation executes when the `shouldBounceOnMount` props is passed.
- [x] Swipe the row and check if the events ran correctly
- [x] Navigate to `SwipeableListViewSimpleExample` and check if the `_animateBounceBack` animation executes when the `shouldBounceOnMount` props is passed.
- [x] Swipe the row and check if the events ran correctly

[GENERAL] [ENHANCEMENT] [Libraries/Experimental/SwipeableRow/SwipeableRow.js] - remove TimerMixin dependency
[GENERAL] [ENHANCEMENT] [RNTester/js/SwipeableListViewSimpleExample.js] - Add bounceFirstRowOnMount to guarantee the SwipeableRow correct behavior.
Pull Request resolved: #21499

Reviewed By: TheSavior

Differential Revision: D10218361

Pulled By: RSNara

fbshipit-source-id: c8e6d5ced4c1237e48bb4c43592016684b2c6360
@danibonilha
Copy link
Contributor

danibonilha commented Oct 10, 2018

I can take that one! (TimersTest)

@danibonilha
Copy link
Contributor

danibonilha commented Oct 11, 2018

@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
Copy link
Contributor

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

@exced
Copy link
Contributor

exced commented Oct 13, 2018

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

@RSNara
Copy link
Contributor

RSNara commented Oct 18, 2018

@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. 😔

facebook-github-bot pushed a commit that referenced this issue Oct 18, 2018
Summary:
Related to #21485 (comment)
Remove createReactClass and TimerMixin from IntegrationTests/TimersTest.js
Pull Request resolved: #21748

Reviewed By: TheSavior

Differential Revision: D10366418

Pulled By: RSNara

fbshipit-source-id: f7e9a1b62a17cd23374997f99dbfe239172b614f
@elicwhite
Copy link
Member Author

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

spongedone

facebook-github-bot pushed a commit that referenced this issue Oct 20, 2018
Summary:
This mixin is no longer used!

closes #21485

Reviewed By: RSNara

Differential Revision: D10451307

fbshipit-source-id: 2315244b2f292cfff71241f95a3afcd9788d74be
kelset pushed a commit that referenced this issue Oct 23, 2018
Summary:
Related to #21485

- Remove TimerMixin from ListView

- All flow tests succeed.
- RNTester: <ListView> iOS (this change should only affect iOS because calculateChildFrames is iOS only)
Show perf monitor, show ListView* screen, start scrolling. UI frame Rate is used at the beginning. When scrolling there is no drop in FPS rate.

TODO: I think a load test would be more relevant:
- Update props multiple times and scroll

[GENERAL] [ENHANCEMENT] [ListView.js] - rm TimerMixin
Pull Request resolved: #21488

Differential Revision: D10219088

Pulled By: RSNara

fbshipit-source-id: 946e4fc1319324c5bf4947a2060b18bebb6fc493
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
…1501)

Summary:
Related to facebook#21485.
Removed TimerMixin from the `RNTester/js/ProgressBarAndroidExample.android.js` since it is currently not used.

- [x] npm run prettier
- [x] npm run flow-check-ios
- [x] npm run flow-check-android

In progress 🙇

[GENERAL] [ENHANCEMENT] [RNTester/js/ProgressBarAndroidExample.android.js] - remove TimerMixin dependency
Pull Request resolved: facebook#21501

Reviewed By: TheSavior

Differential Revision: D10218375

Pulled By: RSNara

fbshipit-source-id: c4c12f65855452bc2485f034a0560afd204512f4
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
Summary:
Related to facebook#21485.
Removed TimerMixin from the `RNTester/js/ProgressViewIOSExample.js` screen since it is currently not used.

- [x] `npm run prettier`
- [x] `npm run flow-check-ios`
- [x] `npm run flow-check-android`
- [x] runtime tests using `ProgressViewIOSExample` on Android and iOS

**RNTester steps**

- [x] Run RNTester.
- [x] Navigate to `ProgressViewIOSExample` and check if the animations are executed correctly and without lag.

[GENERAL] [ENHANCEMENT] [RNTester/js/ProgressViewIOSExample.js] - remove TimerMixin dependency
Pull Request resolved: facebook#21500

Reviewed By: TheSavior

Differential Revision: D10218366

Pulled By: RSNara

fbshipit-source-id: b44a0bbb50f6b0e85f406904131804eace941335
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
Summary:
Related to facebook#21485

- Remove TimerMixin from ListView

- All flow tests succeed.
- RNTester: <ListView> iOS (this change should only affect iOS because calculateChildFrames is iOS only)
Show perf monitor, show ListView* screen, start scrolling. UI frame Rate is used at the beginning. When scrolling there is no drop in FPS rate.

TODO: I think a load test would be more relevant:
- Update props multiple times and scroll

[GENERAL] [ENHANCEMENT] [ListView.js] - rm TimerMixin
Pull Request resolved: facebook#21488

Differential Revision: D10219088

Pulled By: RSNara

fbshipit-source-id: 946e4fc1319324c5bf4947a2060b18bebb6fc493
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
Summary:
Related to facebook#21485.
Removed `TimerMixin` from the `TouchableWithoutFeedback` component since it is currently not used.
Added tests cases for `TouchableWithoutFeedback` to check for any runtime issues.
Pull Request resolved: facebook#21493

Differential Revision: D10219098

Pulled By: RSNara

fbshipit-source-id: d9517b2bd5b72b0450fa864f3556673ae3181552
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
Summary:
Related to facebook#21485.
Removed `TimerMixin` from `ReactContentSizeUpdateTest`.
Pull Request resolved: facebook#21502

Reviewed By: TheSavior

Differential Revision: D10218548

Pulled By: RSNara

fbshipit-source-id: 9a0642e14f8548d8dc9683f0f70353416ed04ae0
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
Summary:
Related to facebook#21485
This PR removes TimerMixin from TouchableOpacity
Pull Request resolved: facebook#21520

Differential Revision: D10223753

Pulled By: RSNara

fbshipit-source-id: fc02077de7e73ee968b7944c0178892825099063
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
Summary:
Related to facebook#21485.
Removed TimerMixin from the SwipeableRow component since it is currently not used.
Added a test case for `SwipeableRow` animation in the `SwipeableListViewSimpleExample`, by adding the `bounceFirstRowOnMount` prop, to check for any runtime issues with the setTimeout method.

- [x] `npm run prettier`
- [x] `npm run flow-check-ios`
- [x] `npm run flow-check-android`
- [x] runtime tests using `SwipeableFlatListExample` on Android and iOS
- [x] runtime tests using `SwipeableListViewSimpleExample` on Android and iOS

**RNTester steps**

- [x] Run RNTester.
- [x] Navigate to `SwipeableFlatListExample` and check if the `_animateBounceBack` animation executes when the `shouldBounceOnMount` props is passed.
- [x] Swipe the row and check if the events ran correctly
- [x] Navigate to `SwipeableListViewSimpleExample` and check if the `_animateBounceBack` animation executes when the `shouldBounceOnMount` props is passed.
- [x] Swipe the row and check if the events ran correctly

[GENERAL] [ENHANCEMENT] [Libraries/Experimental/SwipeableRow/SwipeableRow.js] - remove TimerMixin dependency
[GENERAL] [ENHANCEMENT] [RNTester/js/SwipeableListViewSimpleExample.js] - Add bounceFirstRowOnMount to guarantee the SwipeableRow correct behavior.
Pull Request resolved: facebook#21499

Reviewed By: TheSavior

Differential Revision: D10218361

Pulled By: RSNara

fbshipit-source-id: c8e6d5ced4c1237e48bb4c43592016684b2c6360
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
Summary:
Related to facebook#21485.
Removed TimerMixin from Libraries/Components/TextInput/TextInput.js
Pull Request resolved: facebook#21522

Differential Revision: D10229669

Pulled By: RSNara

fbshipit-source-id: 45de331203eddce06b8fb7ddf4080869c07b6c55
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
Summary:
Related to facebook#21485.
Removed Subscribable.Mixin from the Libraries/Components/ScrollResponder.js

 - [x] npm run prettier
 - [x] npm run flow-check-ios
 - [x] npm run flow-check-android

[GENERAL] [ENHANCEMENT] [Libraries/Components/ScrollResponder.js] - remove Subscribable.Mixin dependency
Pull Request resolved: facebook#21589

Differential Revision: D10275517

Pulled By: RSNara

fbshipit-source-id: 28af7f0944e978609a1b3be05b8a51557e67bc1b
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
Summary:
Related to facebook#21485

Remove SubscribableMixin from SizeFlexibilityUpdateTest

- All flow tests succeed.
- yarn run test-android-instrumentation && CI integration tests

[GENERAL] [ENHANCEMENT] [SizeFlexibilityUpdateTest.js] - rm SubscribableMixin
Pull Request resolved: facebook#21580

Reviewed By: TheSavior

Differential Revision: D10276267

Pulled By: RSNara

fbshipit-source-id: fc89b43099c788cba560c9aaf0819cd5cfab38c3
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
)

Summary:
Related to facebook#21485 (comment)
Remove createReactClass and TimerMixin from IntegrationTests/TimersTest.js
Pull Request resolved: facebook#21748

Reviewed By: TheSavior

Differential Revision: D10366418

Pulled By: RSNara

fbshipit-source-id: f7e9a1b62a17cd23374997f99dbfe239172b614f
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
Summary:
This mixin is no longer used!

closes facebook#21485

Reviewed By: RSNara

Differential Revision: D10451307

fbshipit-source-id: 2315244b2f292cfff71241f95a3afcd9788d74be
@facebook facebook locked as resolved and limited conversation to collaborators Oct 19, 2019
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Oct 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Help Wanted :octocat: Issues ideal for external contributors. JavaScript Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests