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

[createReactClass] remove createReactClass from TimerExample.js #21623

Conversation

peaonunes
Copy link
Contributor

@peaonunes peaonunes commented Oct 10, 2018

Relates to #21581
Removed createdReactClass from TimerExample.js.

Test Plan:

  • yarn run prettier
  • yarn run flow-check-ios
  • yarn run flow-check-android

Run examples on RNTester

  • Run RNTester app, interact with TimerExample examples.

I was expecting to play around and run the tests on RNTester, however I always get stuck while installing the dependencies (it seems I am stucked with this issue #21539. So none of the guides for Mac are working for me :(
Does anyone can help me validating the actual behaviour on RNTester or guide me on fixing my setup?

Release Notes:

[GENERAL] [ENHANCEMENT] [TimerExample.js] - Remove createReactClass

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 10, 2018
Copy link
Contributor

@RSNara RSNara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost ready to merge, but we should make a few changes.

_rafId = (null: ?AnimationFrameID);
_intervalId = (null: ?IntervalID);
_immediateId = (null: ?Object);
_timerFn = (null: ?() => any);
Copy link
Contributor

@RSNara RSNara Oct 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flow won't correctly infer the types of these props based on the types of what you're assigning to them. We should instead do:

_timeId: ?TimeoutID = null;
_rafId: ?AnimationFrameID = null;
_intervalId: ?IntervalID = null;
_immediateId? Object = null;
_timerFn: ?() => any = null;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I see, I remember seeing the way I have done as a suggestion in one of the PRs.
Made your suggested changes on this 66a5b27!


componentWillUnmount() {
componentWillUnmount = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to bind component life-cycle hooks. Since React is responsible for calling these hooks, it can make sure that the value of this is correct inside them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are completely right! Here is the change 18ff98b

Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code analysis results:

  • eslint found some issues.

@@ -11,7 +11,6 @@
'use strict';

var React = require('react');
var createReactClass = require('create-react-class');
var ReactNative = require('react-native');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prettier/prettier: Delete ;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay 28a708f

@@ -11,7 +11,6 @@
'use strict';

var React = require('react');
var createReactClass = require('create-react-class');
var ReactNative = require('react-native');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no-extra-semi: Unnecessary semicolon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wat 28a708f

@peaonunes
Copy link
Contributor Author

I am still not able to run tests with RNTester, @RSNara , how should we proceed?

@RSNara
Copy link
Contributor

RSNara commented Oct 11, 2018

@peaonunes It's cool. I'll verify that this works. 😁

Copy link
Contributor

@RSNara RSNara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@react-native-bot
Copy link
Collaborator

@peaonunes merged commit c96c93e into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Oct 12, 2018
@react-native-bot react-native-bot added the Merged This PR has been merged. label Oct 12, 2018
rozele pushed a commit to microsoft/react-native-windows that referenced this pull request Apr 14, 2019
Summary:
Relates to #21581
Removed `createdReactClass` from `TimerExample.js`.
Pull Request resolved: facebook/react-native#21623

Reviewed By: hramos

Differential Revision: D10341474

Pulled By: RSNara

fbshipit-source-id: b4bf6e07fcf0355c89709809fe9a69e447b44e2f
t-nanava pushed a commit to microsoft/react-native-macos that referenced this pull request Jun 17, 2019
Summary:
Relates to facebook#21581
Removed `createdReactClass` from `TimerExample.js`.
Pull Request resolved: facebook#21623

Reviewed By: hramos

Differential Revision: D10341474

Pulled By: RSNara

fbshipit-source-id: b4bf6e07fcf0355c89709809fe9a69e447b44e2f
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants