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

Removing propTypes from core components #21342

Closed
42 tasks done
elicwhite opened this issue Sep 26, 2018 · 38 comments
Closed
42 tasks done

Removing propTypes from core components #21342

elicwhite opened this issue Sep 26, 2018 · 38 comments
Labels
Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Help Wanted :octocat: Issues ideal for external contributors. JavaScript Resolution: Locked This issue was locked by the bot.

Comments

@elicwhite
Copy link
Member

elicwhite commented Sep 26, 2018

We have been using Flow for our components for a long while and have enjoyed much stricter type safety, enabling us to validate our code at compile time instead of with propTypes at runtime. We want to replace the remaining callsites of propTypes with Flow and deprecate the propType shapes exposed by React Native core. To do this, we need your help!

This is step 1 of our multi step goal. Step 2 can be found here.

There are two classes of things that need to happen to complete this work. The first step is removing references from propTypes in our core components. The following are the individual files that need to be converted / updated. Each file should be converted in its own PR and each file is about the size of a good first issue to learn and get familiar with a part of the React Native codebase. If you’d like to convert one, pick one from the list, comment on this issue that you are interested in converting it, and follow the tips in @empyrical's comment for help successfully updating that file.

Remove callsites from components:

  • Libraries/Components/Button.js -> Delete PropTypes
  • MaskedViewIOS.ios.js -> Delete PropTypes
  • Picker/Picker.js -> Delete PropTypes
  • PickerAndroid.android.js -> Delete PropTypes
  • StaticRenderer.js -> Delete PropTypes
  • StatusBar/StatusBar.js -> Delete PropTypes
  • TabBarIOS/TabBarIOS.ios.js -> Delete PropTypes
  • TabBarIOS/TabBarItemIOS.ios.js -> Convert to Flow Types
  • ViewPagerAndroid.android.js -> Delete PropTypes
  • IncrementalPresenter.js -> Delete PropTypes, leave contextTypes
  • SwipeableFlatList.js -> Delete PropTypes
  • SwipeableListView.js -> Delete PropTypes
  • SwipeableQuickActions.js -> DeletePropTypes (style changed to ViewStyleProp)
  • ElementProperties.js -> Delete PropTypes
  • InspectorPanel -> Delete PropTypes
  • InspectorOverlay.js -> Delete PropTypes
  • SnapshotViewIOS.ios.js -> Delete PropTypes
  • LinkingExample.js -> Convert to Flow Types
  • RNTesterBlock.js -> Delete PropTypes, strengthen Flow Types
  • RNTesterButton.js -> Delete PropTypes
  • RNTesterPage.js -> Delete PropTypes
  • Modal.js -> Convert to Flow Types, slightly bigger task
  • IntegrationTestHarnessTest
  • InputAccessoryView -> no propTypes, but flow types need to get cleaned up

Move and Rename custom propType definitions

For these, I think we should create a new folder, react-native/Libraries/DeprecatedPropTypes. Many of these files have both the Flow definition and the propTypes definition. "Split" means split these into two files, in the example of EdgeInsetsPropType it would mean having an EdgeInsetsPropType.js file that contains the commented flow types, and a DeprecatedEdgeInsetsPropType.js file inside the DeprecatedPropTypes folder. Eventually that folder will become the source for the new repo that we move out.

  • Create a new folder at react-native/Libraries/DeprecatedPropTypes (should be done as part of one of the other tasks)
  • DeprecatedViewPropTypes -> Move, comments to ViewPropTypes
  • EdgeInsetsPropType -> Split
  • PlatformViewPropTypes -> Move
  • TVViewPropTypes -> Split
  • StyleSheetPropType -> Move
  • createStrictShapeTypeChecker -> Move
  • ViewAccessibility -> Split (in progress)
  • ViewStylePropTypes -> Move
  • ImageProps -> Split (in progress)
  • ImageSourcePropType -> Move
  • ImageStylePropTypes -> Move
  • TextPropTypes -> Move
  • ColorPropType -> Move
  • LayoutPropTypes -> Move, comments to StyleSheetTypes
  • ShadowPropTypesIOS -> Move, comments to StyleSheetTypes
  • TransformPropTypes -> Move, comments to StyleSheetTypes
  • PointPropType -> Split
@elicwhite elicwhite added Good first issue Interested in collaborating? Take a stab at fixing one of these issues. JavaScript labels Sep 26, 2018
@elicwhite elicwhite changed the title [Needs Help] Removing propTypes from core components [Need Help] Removing propTypes from core components Sep 26, 2018
@empyrical
Copy link
Contributor

empyrical commented Sep 26, 2018

Tips for removing propTypes from components

To best demonstrate what needs to be done, I'll walk through a component that I converted over (Modal)

Here's before it was converted: https://github.com/facebook/react-native/blob/1151c096dab17e5d9a6ac05b61aacecd4305f3db/Libraries/Modal/Modal.js

And after:

https://github.com/facebook/react-native/blob/master/Libraries/Modal/Modal.js

Before, you can see that it had no flow types for its props at all:

class Modal extends React.Component<Object> {

Its props type was just Object, so it would accept any props as valid. Not very precise!

So we will need to create a flow type to work with:

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

The pipe operator in the brackets mean this type is exact, and the $ReadOnly utility type helps ensure that the values of the props are not manipulated inside of the components.

Then add it to the component like so:

class Modal extends React.Component<Props> {

If the component has state, the syntax is like so:

class Foo extends React.Component<Props, State> {

Before we get to the propTypes, I'd like to point out this part of Modal:

 static contextTypes = {
   rootTag: PropTypes.number,
 };

 // ...

 static childContextTypes = {
   virtualizedList: PropTypes.object,
 };

These usages of the 'prop-types' module aren't being removed yet - so leave them in for now.

For a proptype like an enum:

   /**
    * The `animationType` prop controls how the modal animates.
    *
    * See https://facebook.github.io/react-native/docs/modal.html#animationtype
    */
   animationType: PropTypes.oneOf(['none', 'slide', 'fade']),

It can be ported to Flow like so:

 /**
  * The `animationType` prop controls how the modal animates.
  *
  * See https://facebook.github.io/react-native/docs/modal.html#animationtype
  */
 animationType?: ?('none' | 'slide' | 'fade'),

The question marks mean the property is nullable. Ideally, they should be made nullable where possible.

A great deal of the proptypes are primitive types that can be ported over easily, like:

  • PropTypes.bool -> boolean

  • PropTypes.string -> string

  • PropTypes.number -> number

For PropTypes.arrayOf, you'll want to use $ReadOnlyArray<> rather than Array<>, like so:

   supportedOrientations: PropTypes.arrayOf(
     PropTypes.oneOf([
       'portrait',
       // ... snipped
     ]),
   ),
 // Would become
 supportedOrientations?: ?$ReadOnlyArray<
    | 'portrait'
    // ... snipped
 >,

PropTypes.func is a callback - ideally you should try and fully type the arguments they take, but some of these aren't currently documented so it can be hard to find what their proper types should be. The vast majority of cases, they are SyntheticEvents and you can import them from CoreEventTypes using this syntax:

import type {SyntheticEvent} from  'CoreEventTypes';

And you'd type a callback like this:

 /**
  * The `onRequestClose` callback is called when the user taps the hardware
  * back button on Android or the menu button on Apple TV.
  *
  * This is required on Apple TV and Android.
  *
  * See https://facebook.github.io/react-native/docs/modal.html#onrequestclose
  */
 onRequestClose?: ?(event?: SyntheticEvent<null>) => mixed,

Use mixed rather than void as the return type for these.

I found that the value of the event is null by looking at the native code where the event is fired, and seeing that the value is just nil

 dispatch_block_t completionBlock = ^{
   if (modalHostView.onShow) {
     modalHostView.onShow(nil);
   }
 };

If you're unable to find what the value should be, stub it out using ?Function and ask for help in your pull request.

When you're done, run this command to make sure your code is properly formatted:

yarn prettier
# Or if you don't have yarn
npm run prettier

And run Flow to make sure the types are okay:

yarn flow-check-ios # for *.ios.js files
yarn flow-check-android # for *.android.js files
# Or if you don't have yarn
npm run flow-check-ios # for *.ios.js files
npm run flow-check-android # for *.android.js files

Thank you for helping out, and if you have any questions feel free to ask!

@richardcann
Copy link
Contributor

Im taking on the Libraries/Components/Button.js delete PropTypes conversion

@elicwhite
Copy link
Member Author

@rmcp1g15 Thanks for jumping in! It looks like that one is already checked off (completed). Would you be willing to take a different one?

@empyrical
Copy link
Contributor

@rmcp1g15 That was already converted over, though I realize I forgot this import in it:

const ColorPropType = require('ColorPropType');

@richardcann
Copy link
Contributor

@TheSavior Ok! yeah ill take on StaticRenderer.js and deleting propTypes

@kdastan
Copy link
Contributor

kdastan commented Sep 27, 2018

@TheSavior I'll take ElementProperties.js, InspectorOverlay.js

julioxavierr added a commit to julioxavierr/react-native that referenced this issue Sep 27, 2018
@nissy-dev
Copy link
Contributor

@TheSavior
I'll take MaskedViewIOS.ios.js, ViewPagerAndroid.android.js

@peaonunes
Copy link
Contributor

Just noticed that there is no proptypes in PickerAndroid.android.js, can you confirm?I will take ViewPagerAndroid.android.js instead.

@elicwhite
Copy link
Member Author

PickerAndroid

Looks like PickerAndroid.android.js was taken care of in this commit and I forgot to mark it off. Thanks for flagging!

@danibonilha
Copy link
Contributor

danibonilha commented Sep 27, 2018

@TheSavior Hi, I'll take the creation of DeprecatedPropTypes' folder and split EdgeInsetsPropType

@empyrical
Copy link
Contributor

@danibonilha Just giving you a heads up about this pull request I made:

#21349

@danibonilha
Copy link
Contributor

@empyrical okay, thanks, I'll just split EdgeInsetsPropType then.

@exced
Copy link
Contributor

exced commented Sep 27, 2018

Should ViewStylePropTypes be splitted as well ? (since there's a dependency with PlatformViewPropTypes).

@empyrical
Copy link
Contributor

Yep, that should get split out too

facebook-github-bot pushed a commit that referenced this issue Sep 27, 2018
Summary:
This PR split PointPropType.js into PointPropType.js with Flow definition and  Libraries/DeprecatedPointPropType.js remaining with PropTypes definition.

Related to #21342
Pull Request resolved: #21355

Differential Revision: D10081399

Pulled By: TheSavior

fbshipit-source-id: 2283ff3fbda6b0f525742336f92fd6279250b874
@douglasjunior
Copy link

How to reuse these propTypes now?

In some libraries that I maintain, I do something like this:

import React, { Component } from 'react';
import { Modal } from 'react-native';
import PropTypes from 'prop-types';

class Dialog extends Component {
    ...
} 

Dialog.propTypes = {
   ...Modal.propTypes,
   myCustomProp: PropTypes.string,
}

Any suggestions?

douglasjunior added a commit to douglasjunior/react-native-simple-dialogs that referenced this issue Jan 28, 2019
@elicwhite
Copy link
Member Author

We plan to pull these propTypes out into another npm package that the community can maintain but the goal is that we don’t want to support propTypes, we recommend people use Flow or TypeScript instead.

t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
Summary:
This PR split PointPropType.js into PointPropType.js with Flow definition and  Libraries/DeprecatedPointPropType.js remaining with PropTypes definition.

Related to facebook#21342
Pull Request resolved: facebook#21355

Differential Revision: D10081399

Pulled By: TheSavior

fbshipit-source-id: 2283ff3fbda6b0f525742336f92fd6279250b874
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
Summary:
Related to facebook#21342
Pull Request resolved: facebook#21350

Differential Revision: D10081454

Pulled By: TheSavior

fbshipit-source-id: db27a1f23c643b7d6d73136254eff91625419583
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
Summary:
related facebook#21342

This is a first PR for this repo.
So, if there are any problem, please tell me 🙇

TODO
* delete props types
* apply read only interface

CheckList
 - [x] `yarn prettier`
 - [x] `yarn flow-check-ios`
Pull Request resolved: facebook#21346

Differential Revision: D10081962

Pulled By: TheSavior

fbshipit-source-id: 32387c58f180b9aa5f854e323a4bb29aa73f04c8
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
Summary:
Related to facebook#21342
Pull Request resolved: facebook#21345

Differential Revision: D10081976

Pulled By: TheSavior

fbshipit-source-id: d6a905704fc5c2f10a6a8552f04e9c3feaeb147b
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue 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
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
Summary:
This PR splits EdgeInsetsPropTypes into EdgeInsetsPropTypes with only flow types and DeprecatedEdgeInsetsPropTypes inside DeprecatedProptypes with only PropTypes.

Related to facebook#21342
Pull Request resolved: facebook#21351

Reviewed By: RSNara

Differential Revision: D10081512

Pulled By: TheSavior

fbshipit-source-id: 267a6fbb455e02dd7f2b0f3b59790e96387eaa09
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
Summary:
related facebook#21342

 The `render` function, I was not able to specifically type since the props passed to it may vary. At the moment only `renderRow` function from ListView component is using StaticRenderer, and the type of the renderRow function is `Function`.
Let me know what your thoughts are on this. Thank you
Pull Request resolved: facebook#21348

Differential Revision: D10084990

Pulled By: TheSavior

fbshipit-source-id: a87a8d4976c6ffaf83dc0fddc758869dbc2e2803
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
…21375)

Summary:
related facebook#21342

move TransformPropTypes.js
fix flow error

- [x] yarn prettier
- [x] yarn flow-check-android
- [x] yarn flow-check-ios

All flow checks pass.

[GENERAL] [ENHANCEMENT] [DeprecatedTransformPropTypes.js] - Created.
Pull Request resolved: facebook#21375

Differential Revision: D10095453

Pulled By: TheSavior

fbshipit-source-id: fbf677a000e3c6c0bd31e915dcafbd2d561be6e3
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
…cebook#21372)

Summary:
Related to facebook#21342

- Split TVViewPropTypes
- PlatformViewPropTypes (dependencies) flow types and old prop-type definitions
- ViewStylePropTypes (dependencies) rm prop-type

Flow tests succeed

[GENERAL] [ENHANCEMENT] [TVViewPropTypes.js] - rm prop-types
[GENERAL] [ENHANCEMENT] [PlatformViewPropTypes.android.js] - replace prop-types by Flow
[GENERAL] [ENHANCEMENT] [PlatformViewPropTypes.ios.js] - replace prop-types by Flow
[GENERAL] [ENHANCEMENT] [DeprecatedTVViewPropTypes.js] - old prop-types
Pull Request resolved: facebook#21372

Differential Revision: D10095528

Pulled By: TheSavior

fbshipit-source-id: 4fc52ab194f680f95aabefedcbf119d6897672b7
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
Summary:
related facebook#21342

This is a first PR for this repo.
So, if there are any problem, please tell me 🙇

TODO
* delete props types
* apply read only interface

CheckList
 - [x] `yarn prettier`
 - [x] `yarn flow-check-android`
Pull Request resolved: facebook#21347

Differential Revision: D10095503

Pulled By: TheSavior

fbshipit-source-id: fd1adb5edf19234ae8ae9f3fe732b03a521eb82b
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
…ok#21371)

Summary:
related facebook#21342

TODO
* move ColorPropType.js
* fix flow error

CheckList
 - [x] `yarn prettier`
 - [x] `yarn flow-check-android`
 - [x] `yarn flow-check-ios`

All flow checks pass.

[GENERAL] [ENHANCEMENT] [DeprecatedColorPropType.js] - Created.
Pull Request resolved: facebook#21371

Reviewed By: RSNara

Differential Revision: D10087818

Pulled By: TheSavior

fbshipit-source-id: 48088b441699886eef1fff3aafc2ca6015455006
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
…21379)

Summary:
related facebook#21342

move TransformPropTypes.js
fix flow error

- [x] yarn prettier
- [x] yarn flow-check-android
- [x] yarn flow-check-ios

All flow checks pass.

[GENERAL] [ENHANCEMENT] [DeprecatedShadowPropTypesIOS.js] - Created.
Pull Request resolved: facebook#21379

Differential Revision: D10098750

Pulled By: TheSavior

fbshipit-source-id: f7f2e4bf7b837c00a14b1fbd930d1b29ffb63549
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
…acebook#21380)

Summary:
This PR moves and renames all references of StyleSheetPropType  to DeprecatedStyleSheetPropType
Related to facebook#21342
Pull Request resolved: facebook#21380

Differential Revision: D10098216

Pulled By: TheSavior

fbshipit-source-id: da8d927f87bd37cdabc315e0aa17b6ae208f7124
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
Summary:
Part of: facebook#21342

This PR removes prop types from `SwipeableRow`, and converts it from a `createReactClass` to an ES6 class.
Pull Request resolved: facebook#21386

Differential Revision: D10100555

Pulled By: TheSavior

fbshipit-source-id: ab350546f4fa6f1ed3fdeae07e342890af6d9a22
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
…21384)

Summary:
Part of: facebook#21342

This PR removes the prop types for the components `SwipeableFlatList` and `SwipeableQuickActions`.

The props for `SwipeableFlatList` have been left as not $ReadOnly, because it needs the types in `Libraries/Lists/*` to also be cleaned up. A todo notice has been added.
Pull Request resolved: facebook#21384

Differential Revision: D10099694

Pulled By: TheSavior

fbshipit-source-id: 424b900942c9a7889b664f351f79abee55923430
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
…facebook#21387)

Summary:
related facebook#21342

TODO
* move ImageSourcePropType.js, ImageStylePropTypes.js, TextPropTypes.js
* fix flow error

CheckList
 - [x] `yarn prettier`
 - [x] `yarn flow-check-android`
 - [x] `yarn flow-check-ios`

All flow checks pass.

[GENERAL] [ENHANCEMENT] [DeprecatedImageSourcePropType.js] - Created.
[GENERAL] [ENHANCEMENT] [DeprecatedImageStylePropTypes.js] - Created.
[GENERAL] [ENHANCEMENT] [DeprecatedTextPropTypes.js] - Created.
Pull Request resolved: facebook#21387

Reviewed By: TheSavior

Differential Revision: D10099753

Pulled By: RSNara

fbshipit-source-id: c907af6d1549ee42de1a2e17f278998e8422110f
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
Summary:
Related to facebook#21342
* Renamed ViewStyleProps to DeprecatedViewStyleProps.
* Moved propType declaration to `react-native/Libraries/DeprecatedPropTypes`
*  ImageProps.js: moved propType declarations to DeprecatedImageProps.js.
Pull Request resolved: facebook#21415

Reviewed By: TheSavior

Differential Revision: D10119599

Pulled By: RSNara

fbshipit-source-id: 67674039a88dcd570973c7062f86ebdbd6987d28
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
…ook#21370)

Summary:
related facebook#21342

TODO
* move LayoutPropType.js
* fix flow error

CheckList
 - [x] `yarn prettier`
 - [x] `yarn flow-check-android`
 - [x] `yarn flow-check-ios`

All flow checks pass.

[GENERAL] [ENHANCEMENT] [DeprecatedLayoutPropTypes.js] - Created.
[GENERAL] [ENHANCEMENT] [StyleSheetTypes.js] - add comments.
Pull Request resolved: facebook#21370

Differential Revision: D10099715

Pulled By: RSNara

fbshipit-source-id: d0515fe0d56d9ed2fde50cc0bfb75b63aded1f5d
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
…nTestHarnessTest and InputAccessoryView (facebook#21397)

Summary:
related facebook#21342
Pull Request resolved: facebook#21397

Reviewed By: TheSavior

Differential Revision: D10119623

Pulled By: RSNara

fbshipit-source-id: 16bdb3d5bf90c24b597bbc12fc416a50a0aa2bb1
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
Summary:
Related to facebook#21342
Pull Request resolved: facebook#21392

Reviewed By: TheSavior

Differential Revision: D10129812

Pulled By: RSNara

fbshipit-source-id: 79e900b56eb043452ce8e13e998a9ad8d4443897
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
Summary:
Related to facebook#21342

- Split ImageProps.js: moved propType declarations to DeprecatedImageProps.js
- Renamed ImageProps references to DeprecatedImageProps in Image.ios.js
Pull Request resolved: facebook#21411

Reviewed By: TheSavior

Differential Revision: D10146178

Pulled By: RSNara

fbshipit-source-id: 4db15eaaa8822e834af347d1927991dff1c427cb
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
facebook#21422)

Summary:
This PR splits and renames all references of ViewAccessibility to DeprecatedViewAccessibility
Related to facebook#21342
Pull Request resolved: facebook#21422

Reviewed By: yungsters

Differential Revision: D10132659

Pulled By: RSNara

fbshipit-source-id: 68c371230c69ed37c3e44bf8a36043adb04afc78
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
Summary:
Related to facebook#21342

Move createStrictShapeTypeChecker to DeprecatedCreateStrictShapeTypeChecker.

Reviewed By: hramos

Differential Revision: D10341526

fbshipit-source-id: 30e7f22ae574af620ead9c1a0766f00611b282b6
@facebook facebook locked as resolved and limited conversation to collaborators Oct 16, 2019
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Oct 16, 2019
@elicwhite
Copy link
Member Author

It's been a long journey, but @yungsters just landed 23717c6 which removed the remaining prop-types usages and the DeprecatedPropTypes files that you all helped create from React Native.

We are finally free of prop-types!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Help Wanted :octocat: Issues ideal for external contributors. JavaScript Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests