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.

Sign up for GitHub

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, TimerMixin] remove createReactClass and TImerMixin from TimerTest.js #21748

Closed
wants to merge 3 commits into from
Closed

[createReactClass, TimerMixin] remove createReactClass and TImerMixin from TimerTest.js #21748

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
17cdc0d
Merge pull request #2 from facebook/master
ronal2do Oct 10, 2018
00d4b68
Removing TimerMixin #21485
ronal2do Oct 12, 2018
7cf0f99
remove unused
ronal2do Oct 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
155 changes: 89 additions & 66 deletions IntegrationTests/TimersTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,98 +11,106 @@
'use strict';

const React = require('react');
const createReactClass = require('create-react-class');
const ReactNative = require('react-native');
/* $FlowFixMe(>=0.54.0 site=react_native_oss) This comment suppresses an error
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's get rid of all $FlowFixMes in this file.

* found when Flow v0.54 was deployed. To see the error delete this comment and
* run Flow. */
const TimerMixin = require('react-timer-mixin');

const {StyleSheet, Text, View} = ReactNative;
const {TestModule} = ReactNative.NativeModules;

const TimersTest = createReactClass({
displayName: 'TimersTest',
mixins: [TimerMixin],

_nextTest: () => {},
_interval: -1,

getInitialState() {
return {
count: 0,
done: false,
};
},
const requestAnimationFrame = require('fbjs/lib/requestAnimationFrame');

type Props = $ReadOnly<{||}>;

type State = {|
count: number,
done: boolean,
|};
class TimersTest extends React.Component<Props, State> {
_nextTest = () => {};
_timeoutID: ?TimeoutID = null;
// $FlowFixMe
_intervalId: ?IntervalID = -1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TheSavior @RSNara

  // $FlowFixMe
_intervalId: ?IntervalID = -1;

I've had issues with Flow here and on the contructor.displayName
because null or undefined is
incompatible with IntervalID,
and number is incompatible with IntervalID

Copy link
Contributor

Choose a reason for hiding this comment

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

I commented below how we can fix this.

_immediateId: ?Object = null;

state = {
count: 0,
done: false,
};

componentDidMount() {
this.setTimeout(this.testSetTimeout0, 1000);
},
this._timeoutID = setTimeout(this.testSetTimeout0, 1000);
}

testSetTimeout0() {
this.setTimeout(this.testSetTimeout1, 0);
},
this._timeoutID = setTimeout(this.testSetTimeout1, 0);
}

testSetTimeout1() {
this.setTimeout(this.testSetTimeout50, 1);
},
this._timeoutID = setTimeout(this.testSetTimeout50, 1);
}

testSetTimeout50() {
this.setTimeout(this.testRequestAnimationFrame, 50);
},
this._timeoutID = setTimeout(this.testRequestAnimationFrame, 50);
}

testRequestAnimationFrame() {
this.requestAnimationFrame(this.testSetInterval0);
},
requestAnimationFrame(this.testSetInterval0);
}

testSetInterval0() {
this._nextTest = this.testSetInterval20;
this._interval = this.setInterval(this._incrementInterval, 0);
},
this._intervalId = setInterval(this._incrementInterval, 0);
}

testSetInterval20() {
this._nextTest = this.testSetImmediate;
this._interval = this.setInterval(this._incrementInterval, 20);
},
this._intervalId = setInterval(this._incrementInterval, 20);
}

testSetImmediate() {
this.setImmediate(this.testClearTimeout0);
},
this._immediateId = setImmediate(this.testClearTimeout0);
}

testClearTimeout0() {
const timeout = this.setTimeout(() => this._fail('testClearTimeout0'), 0);
this.clearTimeout(timeout);
const timeout = setTimeout(() => this._fail('testClearTimeout0'), 0);
clearTimeout(timeout);
this.testClearTimeout30();
},
}

testClearTimeout30() {
const timeout = this.setTimeout(() => this._fail('testClearTimeout30'), 30);
this.clearTimeout(timeout);
this.setTimeout(this.testClearMulti, 50);
},
const timeout = setTimeout(() => this._fail('testClearTimeout30'), 30);
clearTimeout(timeout);
this._timeoutID = setTimeout(this.testClearMulti, 50);
}

testClearMulti() {
const fails = [];
fails.push(this.setTimeout(() => this._fail('testClearMulti-1'), 20));
fails.push(this.setTimeout(() => this._fail('testClearMulti-2'), 50));
const delayClear = this.setTimeout(
() => this._fail('testClearMulti-3'),
50,
fails.push(
(this._timeoutID = setTimeout(() => this._fail('testClearMulti-1'), 20)),
);
fails.push(
(this._timeoutID = setTimeout(() => this._fail('testClearMulti-2'), 50)),
);
const delayClear = setTimeout(() => this._fail('testClearMulti-3'), 50);
fails.push(
(this._timeoutID = setTimeout(() => this._fail('testClearMulti-4'), 0)),
);
fails.push(
(this._timeoutID = setTimeout(() => this._fail('testClearMulti-5'), 10)),
);
fails.push(this.setTimeout(() => this._fail('testClearMulti-4'), 0));
fails.push(this.setTimeout(() => this._fail('testClearMulti-5'), 10));

fails.forEach(timeout => this.clearTimeout(timeout));
this.setTimeout(() => this.clearTimeout(delayClear), 20);
fails.forEach(timeout => clearTimeout(timeout));
this._timeoutID = setTimeout(() => clearTimeout(delayClear), 20);

this.setTimeout(this.testOrdering, 50);
},
this._timeoutID = setTimeout(this.testOrdering, 50);
}

testOrdering() {
// Clear timers are set first because it's more likely to uncover bugs.
let fail0;
this.setImmediate(() => this.clearTimeout(fail0));
fail0 = this.setTimeout(
this._immediateId = setImmediate(() => clearTimeout(fail0));
fail0 = setTimeout(
() =>
this._fail(
'testOrdering-t0, setImmediate should happen before ' +
Expand All @@ -111,62 +119,77 @@ const TimersTest = createReactClass({
0,
);
let failAnim; // This should fail without the t=0 fastpath feature.
this.setTimeout(() => this.cancelAnimationFrame(failAnim), 0);
failAnim = this.requestAnimationFrame(() =>
this._timeoutID = setTimeout(() => cancelAnimationFrame(failAnim), 0);
failAnim = requestAnimationFrame(() =>
this._fail(
'testOrdering-Anim, setTimeout 0 should happen before ' +
'requestAnimationFrame',
),
);
let fail25;
this.setTimeout(() => {
this.clearTimeout(fail25);
this._timeoutID = setTimeout(() => {
clearTimeout(fail25);
}, 20);
fail25 = this.setTimeout(
fail25 = setTimeout(
() =>
this._fail(
'testOrdering-t25, setTimeout 20 should happen before ' +
'setTimeout 25',
),
25,
);
this.setTimeout(this.done, 50);
},
this._timeoutID = setTimeout(this.done, 50);
}

done() {
this.setState({done: true}, () => {
TestModule.markTestCompleted();
});
},
}

componentWillUnmount() {
if (this._timeoutID != null) {
clearTimeout(this._timeoutID);
}
if (this._immediateId != null) {
clearImmediate(this._immediateId);
}
if (this._intervalId != null) {
clearInterval(this._intervalId);
}
}

render() {
return (
<View style={styles.container}>
<Text>
{this.constructor.displayName + ': \n'}
{/* $FlowFixMe(>=0.54.0 site=react_native_fb,react_native_oss) This
* comment suppresses an error found when Flow v0.54 was deployed.
* To see the error delete this comment and run Flow. */
this.constructor.displayName + ': \n'}
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, the displayName is added on to our components in a babel transform. Flow doesn't know anything about that, so that's probably why it's raising this error.

I think there're three fixes:

  1. Change this.constructor.displayName to this.constructor.name. (Both should be equivalent).
  2. Redundantly define the displayName static property in this class.
  3. Hardcode 'TimerTest' instead of this.constructor.displayName.

Your call. 😄

Intervals: {this.state.count + '\n'}
{this.state.done ? 'Done' : 'Testing...'}
</Text>
</View>
);
},
}

_incrementInterval() {
if (this.state.count > 3) {
throw new Error('interval incremented past end.');
}
if (this.state.count === 3) {
this.clearInterval(this._interval);
clearInterval(this._intervalId);
Copy link
Contributor

Choose a reason for hiding this comment

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

The signature of clearInterval is this:

function clearInterval(interval?: IntervalID) {

So clearInterval expects an argument that is either an IntervalID, undefined, or not present at all. You're passing in a value that's ?IntervalID, which is equivalent to IntervalID, undefined, or null. (The bottom line is that clearInterval does not accept null as an argument). Therefore, If you guard this function call with a null check, your flow error should go away.

if (this.state.count === 3) {
  if (this._intervalId != null) {
    clearInterval(this._intervalId);
  }
}

this.setState({count: 0}, this._nextTest);
return;
}
this.setState({count: this.state.count + 1});
},
}

_fail(caller: string): void {
throw new Error('_fail called by ' + caller);
},
});
}
}

const styles = StyleSheet.create({
container: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's get rid of:

TimersTest.displayName = 'TimersTest';

If we want to define static properties on TimerTest, we should do it using the static keyword inside the TimerTest class. That way, flow will be able to analyze the static property.

Expand Down