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

Deprecate and eventually remove propTypes from core #29

Closed
TheSavior opened this Issue Sep 22, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@TheSavior

TheSavior commented Sep 22, 2018

The problem

Enforcing React prop type safety via propTypes is dubious at best. We've been using Flow for a much stricter enforcement of these props for quite a while. Unfortunately, much of our codebase existed before we typed our React Components and thus a bunch of stuff is still using PropTypes or some mix thereof. For a long time we've had files that export propTypes as well as Flow types and we tried to keep them in sync. However, we want to discourage the usage of propTypes in favor of static typing and only have one source of truth for the APIs of these components. That source of truth is Flow.

An Aside

We have a couple of projects that want to use these Flow types as the source of truth. For example, using them to codegen native code for each component and ensuring the native components supports props of the same type.

The proposal

Deprecate and remove propTypes from core RN!

Right now there are a couple of different ways propTypes are being used.

  1. Exposed as APIs from react-native’s main export
  2. Components that still use createReactClass
  3. Components that no longer use createReactClass but still define propTypes

For open source, since the propType APIs from our main export have been public for a long time, we probably need to actually move these out into a deprecated repo instead of just deleting them. Before we can move them out, we need to fully decouple the propTypes from the rest of the repo.

It is a lot of work to do this, but none of it is very challenging. I'm hoping we can label these individual pieces of work as "Good First Task" and enlist the help of the community. This is the order of changes I think need to be made:

Remove callsites from components:

For a list of files, see the issue here: facebook/react-native#21342

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. There are two reasons I'm proposing renaming to have a Deprecated prefix. One is that Haste requires files to have different names. The other is that it will catch our internal usage of these modules. We have an internal lint rule that will start detecting callsites and yelling at teams to remove these requires (because they will include Deprecated in the name).

For a list of files, see the issue here: facebook/react-native#21342

Once these modules are all separated, we can move them to a new repo, perhaps something like facebookarchive/react-native-deprecated-prop-types. react-native can add a devDependency to that repo for a while to support the remaining callsites in core (there are still a few createReactClass callsites that do complicated things with mixins which will take much more work to remove.

Once this new repo exists, we can add deprecation warnings to react-native-implementation.js:

get ColorPropType() {
  // some deprecation warning, tell people to change their requires
  return require('react-native-deprecated-prop-types').ColorPropType;
},
get EdgeInsetsPropType() {
   // some deprecation warning, tell people to change their requires
  return require('react-native-deprecated-prop-types/lib/EdgeInsetsPropType.js');
},
get PointPropType() {
  // some deprecation warning, tell people to change their requires
  return require('react-native-deprecated-prop-types/lib/PointPropType.js');
},
get ViewPropTypes() {
  // some deprecation warning, tell people to change their requires
  return require('react-native-deprecated-prop-types/lib/ViewPropTypes.js');
},

I'm specifically looking at this as a call for help. Both in getting all this work done, and in reviewing and landing PRs. Are there any concerns with this approach or better/easier ways we can go about getting this all done?

@TheSavior TheSavior changed the title from Deprecate and eventually remove propTypes from Core to Deprecate and eventually remove propTypes from core Sep 22, 2018

@empyrical

This comment has been minimized.

Member

empyrical commented Sep 23, 2018

Made a stab at this, looking for reviews: facebook/react-native#21278

Also created a <Timer /> component as an alternative to TimerMixin.

Looking at all the classes with mixins, it looks like NativeMethodsMixin will be the most complex to try and make an ES6+ alternative for and will require some brainstorming.

I definitely think that it's worth it to try and convert the last of the createReactClasses while we're at it, because when that happens I notice the flow type checking around them seems to get better

@empyrical

This comment has been minimized.

Member

empyrical commented Sep 23, 2018

Modal.js as well: facebook/react-native#21279

@TheSavior

This comment has been minimized.

TheSavior commented Sep 24, 2018

@empyrical, we have already converted most of the modules using NativeMethodsMixin. We converted them to using React.forwardRef. You can see this commit as an example: facebook/react-native@64d9c48

I completed all of the standard components but didn't convert the ones that only had .ios.js or .android.js files. Those still need to get converted:

screen shot 2018-09-23 at 6 25 42 pm

facebook-github-bot added a commit to facebook/react-native that referenced this issue Sep 24, 2018

Button: Remove PropTypes (#21280)
Summary:
Part of: react-native-community/discussions-and-proposals#29

This PR removes the `prop-types` from the `Button` component, and cleans up its flow type definitions.
Pull Request resolved: #21280

Differential Revision: D10007108

Pulled By: TheSavior

fbshipit-source-id: 6206f7e8aab5b56abc5e8e0790a1020494eb2bf0

facebook-github-bot added a commit to facebook/react-native that referenced this issue Sep 24, 2018

Picker: Remove PropTypes (#21281)
Summary:
Part of: react-native-community/discussions-and-proposals#29

This pull request removes all PropTypes from the various files for `Picker` and cleans up their flow types.
Pull Request resolved: #21281

Differential Revision: D10007224

Pulled By: TheSavior

fbshipit-source-id: 5b8b7918cc918dd77e7ab27c9e3921ffbeb4ff73

facebook-github-bot added a commit to facebook/react-native that referenced this issue Sep 24, 2018

SnapshotViewIOS: Remove PropTypes (#21294)
Summary:
Part of: react-native-community/discussions-and-proposals#29

This PR removes all PropTypes from `SnapshotViewIOS`, and fills out the flow types for its event callbacks.
Pull Request resolved: #21294

Differential Revision: D10011659

Pulled By: TheSavior

fbshipit-source-id: 28bfa0ab58c0655f9b905d3cb6530b57166c67f9

facebook-github-bot added a commit to facebook/react-native that referenced this issue Sep 24, 2018

StatusBar: Remove PropTypes (#21293)
Summary:
Part of: react-native-community/discussions-and-proposals#29

This PR removes the remaining PropTypes from `StatusBar` and moves its flowtypes to its own definition.
Pull Request resolved: #21293

Differential Revision: D10012963

Pulled By: TheSavior

fbshipit-source-id: 7fb4e416eb49e7860809a3e2aaf157590908687d

facebook-github-bot added a commit to facebook/react-native that referenced this issue Sep 25, 2018

TabBarIOS: Remove PropTypes (#21315)
Summary:
Part of: react-native-community/discussions-and-proposals#29

This PR removes the prop types from the TabBarIOS files, and cleans up their flow types.
Pull Request resolved: #21315

Reviewed By: TheSavior

Differential Revision: D10031191

Pulled By: rsnara

fbshipit-source-id: 50dc26b858ea5b065a3934080af7e6b0e36c7f46

facebook-github-bot added a commit to facebook/react-native that referenced this issue Sep 26, 2018

RNTester: Remove all but one instance of PropTypes (#21321)
Summary:
Part of: react-native-community/discussions-and-proposals#29

This PR removes all but one instance of PropTypes in `RNTester`. The last remaining conversion is `CameraRollView`, which I will do in a separate PR.
Pull Request resolved: #21321

Differential Revision: D10041809

Pulled By: TheSavior

fbshipit-source-id: c03b1ce5ad640ae59ae6240a3b6c13581345b5a3
@TheSavior

This comment has been minimized.

TheSavior commented Sep 26, 2018

Seems like this proposal is not controversial, I have posted an issue asking for help here: facebook/react-native#21342. @empyrical is going to post a set of instructions for how people can update these and then we can tweet out a link to that issue.

@kelset

This comment has been minimized.

Collaborator

kelset commented Sep 27, 2018

Thanks for all the work you folks have been doing on this, it's awesome to see all these commits land so quickly 👏

Btw since you opened an in issue in the main repo, do you think we can close this now?

@TheSavior TheSavior closed this Sep 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment