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

Remove PropTypes from IncrementalPresenter #21343

Closed
wants to merge 4 commits into from

Conversation

julioxavierr
Copy link
Contributor

Related to #21342

@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 Sep 27, 2018
@pull-bot
Copy link

Warnings
⚠️

📋 Test Plan - This PR appears to be missing a Test Plan.

⚠️

📋 Release Notes - This PR appears to be missing Release Notes.

Generated by 🚫 dangerJS

@react-native-bot react-native-bot added Missing Test Plan This PR appears to be missing a test plan. Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. labels Sep 27, 2018
Copy link
Contributor

@empyrical empyrical left a comment

Choose a reason for hiding this comment

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

Hey! Thanks for the PR.

There are a few quick tweaks that should be made to this -

The props should be made read-only and exact, like this:

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

The callbacks should be cleaned up, so that their return type is mixed rather than void.

The type for event on onLayout should also be LayoutEvent, which you can import from:

import type {LayoutEvent} from 'CoreEventTypes';

You also need to add children as a type to Props:

children?: React.Node,

@julioxavierr
Copy link
Contributor Author

Just did the request changes, @empyrical. Thanks for the advise.

@empyrical
Copy link
Contributor

Looks like there's a flow failure here:

Cannot create `View` element because mixed [1] is incompatible with undefined [2] in the return value of property
`onLayout`.

   Libraries/Experimental/IncrementalPresenter.js:85:9
               v----
   85|         <View
   86|           children={this.props.children}
   87|           ref="view"
   88|           style={style}
   89|           onLayout={this.props.onLayout}
   90|         />
               -^

References:
   Libraries/Experimental/IncrementalPresenter.js:38:38
   38|   onLayout?: (event: LayoutEvent) => mixed,
                                            ^^^^^ [1]
   Libraries/Components/View/ViewPropTypes.js:31:39
   31|   onLayout?: ?(event: LayoutEvent) => void,
                                             ^^^^ [2]

This is because the onLayout type in ViewPropTypes.js needs to be fixed so it returns mixed, rather than void. Then your PR should pass tests! 👍

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.

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

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.

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

@react-native-bot
Copy link
Collaborator

@julioxavierr merged commit 0625c34 into facebook:master.


Once this commit is added to a release, you will see the corresponding version tag below the description at 0625c34. If the commit has a single master tag, it is not yet part of a release.

@facebook facebook locked as resolved and limited conversation to collaborators Sep 27, 2018
@react-native-bot react-native-bot added the Merged This PR has been merged. label Sep 27, 2018
@elicwhite
Copy link
Member

Thanks for contributing!

@julioxavierr julioxavierr deleted the patch-1 branch September 27, 2018 23:46
t-nanava pushed a commit to microsoft/react-native-macos that referenced this pull request Jun 17, 2019
Summary:
Related to facebook#21342
Pull Request resolved: facebook#21343

Reviewed By: RSNara

Differential Revision: D10080219

Pulled By: TheSavior

fbshipit-source-id: 3a9108208fe6aaa7a30b99f24ceef03e884ec48a
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. Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. Missing Test Plan This PR appears to be missing a test plan.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants