-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
[createReactClass] remove createReactClass from TimerExample.js #21623
Conversation
There was a problem hiding this 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.
RNTester/js/TimerExample.js
Outdated
_rafId = (null: ?AnimationFrameID); | ||
_intervalId = (null: ?IntervalID); | ||
_immediateId = (null: ?Object); | ||
_timerFn = (null: ?() => any); |
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
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!
RNTester/js/TimerExample.js
Outdated
|
||
componentWillUnmount() { | ||
componentWillUnmount = () => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this 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'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prettier/prettier: Delete ;
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wat 28a708f
I am still not able to run tests with RNTester, @RSNara , how should we proceed? |
@peaonunes It's cool. I'll verify that this works. 😁 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏼
There was a problem hiding this 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.
@peaonunes merged commit c96c93e into |
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
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
Relates to #21581
Removed
createdReactClass
fromTimerExample.js
.Test Plan:
Run examples on RNTester
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