-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
[FlowCleanup] InspectorPanel -> Delete PropTypes #21392
Conversation
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
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.
eslint
found some issues. You may run yarn prettier
or npm run prettier
to fix these.
...ViewProps, | ||
devtoolsIsOpen?: ?boolean, | ||
inspecting?: ?boolean, | ||
setInspecting?: ?Function, |
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.
Unclear type. Using any
, Object
, Function
, $Subtype<...>
, or $Supertype<...>
types is not safe! (unclear-type
)
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.
This should be:
setInspecting?: ?(val: boolean) => void
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.
edit: never mind this, missed the custom button component
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.
@empyrical Hah! You made the same mistake I did. :P
More of a reason to rename Button
to InspectorPanelButton
.
inspecting?: ?boolean, | ||
setInspecting?: ?Function, | ||
perfing?: ?boolean, | ||
setPerfing?: ?Function, |
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.
Unclear type. Using any
, Object
, Function
, $Subtype<...>
, or $Supertype<...>
types is not safe! (unclear-type
)
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.
This should be:
setPerfing?: ?(val: boolean) => void
perfing?: ?boolean, | ||
setPerfing?: ?Function, | ||
touchTargeting?: ?boolean, | ||
setTouchTargeting?: ?Function, |
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.
Unclear type. Using any
, Object
, Function
, $Subtype<...>
, or $Supertype<...>
types is not safe! (unclear-type
)
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.
This should be:
setTouchTargeting?: ?(val: boolean) => void
touchTargeting?: ?boolean, | ||
setTouchTargeting?: ?Function, | ||
networking?: ?boolean, | ||
setNetworking?: ?Function, |
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.
Unclear type. Using any
, Object
, Function
, $Subtype<...>
, or $Supertype<...>
types is not safe! (unclear-type
)
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.
This should be:
setNetworking?: ?(val: boolean) => void
}; | ||
|
||
class Button extends React.Component<$FlowFixMeProps> { | ||
type ButtonProps = $ReadOnly<{| | ||
...ViewProps, |
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.
Unclear type. Using any
, Object
, Function
, $Subtype<...>
, or $Supertype<...>
types is not safe! (unclear-type
)
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.
We don't need to spread ViewProps
into ButtonProps
. The render method of Button
only uses onClick
, pressed
, and title
.
Generated by 🚫 dangerJS |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
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.
This is awesome!
But, I'd like to request a few changes. :)
...ViewProps, | ||
devtoolsIsOpen?: ?boolean, | ||
inspecting?: ?boolean, | ||
setInspecting?: ?Function, |
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.
This should be:
setInspecting?: ?(val: boolean) => void
inspecting?: ?boolean, | ||
setInspecting?: ?Function, | ||
perfing?: ?boolean, | ||
setPerfing?: ?Function, |
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.
This should be:
setPerfing?: ?(val: boolean) => void
perfing?: ?boolean, | ||
setPerfing?: ?Function, | ||
touchTargeting?: ?boolean, | ||
setTouchTargeting?: ?Function, |
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.
This should be:
setTouchTargeting?: ?(val: boolean) => void
touchTargeting?: ?boolean, | ||
setTouchTargeting?: ?Function, | ||
networking?: ?boolean, | ||
setNetworking?: ?Function, |
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.
This should be:
setNetworking?: ?(val: boolean) => void
}; | ||
|
||
class Button extends React.Component<$FlowFixMeProps> { | ||
type ButtonProps = $ReadOnly<{| | ||
...ViewProps, |
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.
We don't need to spread ViewProps
into ButtonProps
. The render method of Button
only uses onClick
, pressed
, and title
.
title?: ?string, | ||
|}>; | ||
|
||
class Button extends React.Component<ButtonProps> { |
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.
nit: Can we rename this component to InspectorPanelButton
? This current naming is super misleading. When I was reading through this code initially, I thought that the <Button/>
used in InspectorPanel
was the button component from 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.
Same here, got confused by that too. I think it would be worth renaming
// import type {SyntheticEvent} from 'CoreEventTypes'; | ||
|
||
type Props = $ReadOnly<{| | ||
...ViewProps, |
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.
It looks like the render method of InspectorPanel
doesn't use any the props from View
. So it's unnecessary to spread ViewProps
into Props
.
}; | ||
|
||
class Button extends React.Component<$FlowFixMeProps> { | ||
type ButtonProps = $ReadOnly<{| |
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.
nit: Rename to InspectorPanelButtonProps
.
class InspectorPanel extends React.Component<$FlowFixMeProps> { | ||
import type {ViewProps} from 'ViewPropTypes'; | ||
// Will add this once I get params for the functions | ||
// import type {SyntheticEvent} from 'CoreEventTypes'; |
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.
I'm afraid I don't follow these comments. Should they be here?
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.
In my instructions, I suggested that people could stub out functions types with ?Function
and ask for help getting the function signatures in their pull request if they needed help, and looks like you did just that here 👍
@RSNara Thanks for the help, I will come out with another commit filling out the functions. |
}; | ||
|
||
class Button extends React.Component<$FlowFixMeProps> { | ||
type InspectorPanelButtonProps = $ReadOnly<{| | ||
onClick?: ?Function, |
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 I need a similar params/return type here.
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're right. This should also take the same signature:
onClick?: ?(val: boolean) => void
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.
Now that I think about it more, a lot of these types are unnecessarily optional. For example, I think onClick
should not be optional, since we assume (in the implementation of this component) that it's a function. Do you mind making these optional properties, that don't need to be optional, non-optional?
class InspectorPanel extends React.Component<$FlowFixMeProps> { | ||
type Props = $ReadOnly<{| | ||
devtoolsIsOpen?: ?boolean, | ||
inspecting?: ?boolean, |
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.
Circle CI errors indicate that we need to strict type it rather than ?boolean
. Do we want strict type checking or nullable type checking? or maybe both. I think this applies to all the boolean values when we are doing:
if(value) then something
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.
Does the following work?
if (value === true) {
// stuff
}
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.
Yes. But these props can be either boolean or undefined. I don't know how to write for both? ?boolean
should work, but I get this error:
Sketchy null check on boolean [1] which is potentially false. Perhaps you meant to check for null or undefined [2]?
(`sketchy-null-bool`)`
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... There's only one use of <InspectorPanel>
in the codebase:
<InspectorPanel
devtoolsIsOpen={!!this.state.devtoolsAgent}
inspecting={this.state.inspecting}
perfing={this.state.perfing}
setPerfing={this.setPerfing.bind(this)}
setInspecting={this.setInspecting.bind(this)}
inspected={this.state.inspected}
hierarchy={this.state.hierarchy}
selection={this.state.selection}
setSelection={this.setSelection.bind(this)}
touchTargeting={Touchable.TOUCH_TARGET_DEBUG}
setTouchTargeting={this.setTouchTargeting.bind(this)}
networking={this.state.networking}
setNetworking={this.setNetworking.bind(this)}
/>
Lets make the inspecting
, setInspecting
, perfing
, setPerfing
, selection
, setSelection
props non-optional. That should fix our problems.
…to primitive types (can be exported to child components)
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.
eslint
found some issues. You may run yarn prettier
or npm run prettier
to fix these.
const ScrollView = require('ScrollView'); | ||
const StyleSheet = require('StyleSheet'); | ||
const Text = require('Text'); | ||
const TouchableHighlight = require('TouchableHighlight'); | ||
const View = require('View'); | ||
|
||
class InspectorPanel extends React.Component<$FlowFixMeProps> { | ||
import type {ViewStyleProp} from 'StyleSheet'; |
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.
Unclear type. Using any
, Object
, Function
, $Subtype<...>
, or $Supertype<...>
types is not safe! (unclear-type
)
fileName?: string, | ||
lineNumber?: number, | ||
}, | ||
}, |
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.
I think we should be more strict and use exact object types here, so:
inspected?: ?{|
style?: ?ViewStyleProp,
frame?: ?{|
top?: ?number,
left?: ?number,
width?: ?number,
height: ?number,
|},
source?: ?{|
fileName?: string,
lineNumber?: number,
|},
|},
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.
Cool. Also, I found these types from other components. So should we export these types on those components and reference them here? There are many components in Inspector which needs to be converted to flow types.
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.
Yeah, that sounds like a great idea! I can see there being an InspectorTypes
module under Libraries/Inspector
that contains shared types for the modules in that folder. But I think it may be too much to make that refactor in this diff.
Would you like to tackle this in another PR? For now, we can just copy-paste the frame type here.
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.
Sure. I'll finish up with this today and work on the Inspector folder next week.
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.
Sweet! Let me merge this. :)
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.
@dkaushik95 merged commit c870533 into |
Summary: Related to facebook#21342 Pull Request resolved: facebook#21392 Reviewed By: TheSavior Differential Revision: D10129812 Pulled By: RSNara fbshipit-source-id: 79e900b56eb043452ce8e13e998a9ad8d4443897
Related to #21342
Test Plan:
There are 4 functions which I have stubbed as
?Function
and I need some help with the parameters and/or return types (if the return type isvoid
, I will put it asmixed
so no need to mention that.)The code is properly formatted.
I also ran Flow to make sure I don't get errors for this file.
Release Notes
Help reviewers and the release process by writing your own release notes. See below for an example.
[CATEGORY] [TYPE] [LOCATION] - Message