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

Update $FlowFixMees #806

Merged
merged 9 commits into from
Dec 12, 2019
Merged

Update $FlowFixMees #806

merged 9 commits into from
Dec 12, 2019

Conversation

mondoreale
Copy link
Collaborator

Another one of these. As I promised, a whole bunch of $FlowFixMe's gone. The ones that stayed are… well, even I don't know wtf does flow want. 🐽

Also, I added an eslint rule for $FlowFixMe comments. They need explanation from now on. A simple // $FlowFixMe won't do. Hope that's helpful.

@mondoreale mondoreale requested review from timoxley and juhah December 11, 2019 16:48
PortExport.styles = styles

export default PortExport
export default (React.memo(Port): ComponentType<Props>)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

General rule for things like memo or forwardRef is that the exporter has to let others know what kind of thing they're importing. ComponentType<Props> is fine for most cases.

@@ -78,7 +80,7 @@ class HistoryView extends Component<Props, State> {
}

mounted = false
fileUploadRef = React.createRef()
fileUploadRef: Ref<Dropzone> = React.createRef()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one is interesting. We have to declare what does FileUpload assign to this ref. In this case it's a Dropzone instance.

Copy link
Contributor

@timoxley timoxley left a comment

Choose a reason for hiding this comment

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

lgtm 👍

app/src/editor/shared/components/Map/Map.jsx Show resolved Hide resolved
app/src/marketplace/modules/communityProduct/services.js Outdated Show resolved Hide resolved

if (!resend) {
delete options.resend
}
Copy link
Contributor

Choose a reason for hiding this comment

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

sure, this is probably clearer anyway

@@ -122,7 +124,7 @@ const StreamList = () => {
setSort,
resetFilter,
} = useFilterSort(sortOptions)
const [dialogTargetStream, setDialogTargetStream] = useState(undefined)
const [dialogTargetStream, setDialogTargetStream]: TargetStreamSetter = useState(null)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish we could just stop using null but I see the rest of this code uses null so 🤷‍♀

@@ -155,6 +155,7 @@ export function SecurityIcon({
}

export function SecurityView(props: Props) {
// $FlowFixMe `updateEditStream` not in OwnProps or StateProps.
Copy link
Contributor

Choose a reason for hiding this comment

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

@mondoreale any idea what the problem is here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No idea. That's the main "wtf" here. I left those out.

)
.reduce((result, transaction: TransactionEntity) => (
result.includes(transaction.productId) ? result : [...result, (transaction.productId || '')]
), [])
Copy link
Contributor

Choose a reason for hiding this comment

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

Not important really but this reduce should probably go away in favour of using aSet to do the deduping:

Array.from(new Set(productsToFetch.map(({ productId }) => productId))

I guess most of them explanations are gonna be “wtf?”
or “Yeah, flow, very funny!”. Still, better than nothing!
The react-hooks deps likes this one, actually. eslint
by itself doesn’t care.
Copy link
Contributor

@juhah juhah left a comment

Choose a reason for hiding this comment

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

Good fixes 🎉

@mondoreale mondoreale merged commit 196b172 into development Dec 12, 2019
@mondoreale mondoreale deleted the update-flowfixmees branch December 12, 2019 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants