Skip to content

Remove PropTypes from IncrementalPresenter#21343

Closed
julioxavierr wants to merge 4 commits into
react:masterfrom
julioxavierr:patch-1
Closed

Remove PropTypes from IncrementalPresenter#21343
julioxavierr wants to merge 4 commits into
react:masterfrom
julioxavierr:patch-1

Conversation

@julioxavierr

Copy link
Copy Markdown
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
Copy Markdown
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

@empyrical empyrical left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

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

@empyrical

Copy link
Copy Markdown
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! 👍

@facebook-github-bot facebook-github-bot left a comment

Copy link
Copy Markdown
Contributor

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.

@facebook-github-bot facebook-github-bot left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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.

@react react 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
Copy Markdown
Contributor

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 react#21342
Pull Request resolved: react#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