React-native: Fix validation on changing propTypes. JS -> Native

[christopherdro]

Opening this to figure out the best way to fix issues #2397 and #4521.

What's happening with both of these native components is that the propType value passed in gets altered before being passed to the native component.

Here is where that change happens with DatePickerIOS & MapView.
In both instances the prop value changes to an integer and conflicts with the original type defined.

Now we can use the nativeOnly fields and set whichever prop to true that we wish to be ignored but the props never change and get inherited from the parent. (See here), until finally reaching ReactElementValidator where the warnings are being returned from.

The docs for using nativeOnly state:

Since you don't want these native only properties to be part of the API, you don't want to put them in propTypes, _but if you don't you'll get an error_. The solution is simply to call them out via the nativeOnly option.

Does that mean that if we have the fields set in nativeOnly, that we shouldn't be defining them as a propType? Maybe they should've been removed and only exists since the documentation for the component is extracted from the source?

Thoughts?

[facebook-github-bot]

Hey christopherdro, thanks for reporting this issue!

React Native, as you've probably heard, is getting really popular and truth is we're getting a bit overwhelmed by the activity surrounding it. There are just too many issues for us to manage properly.

• If this is a feature request or a bug that you would like to be fixed by the team, please report it on Product Pains. It has a ranking feature that lets us focus on the most important issues the community is experiencing.
• If you don't know how to do something or not sure whether some behavior is expected or a bug, please ask on StackOverflow with the tag react-native or for more real time interactions, ask on Discord in the #react-native channel.
• We welcome clear issues and PRs that are ready for in-depth discussion; thank you for your contributions!

[mkonicek]

Thanks for the awesome summary! Feel free to post about things like this in the contributors group so it gets visibility.

[nicklockwood]

This should be straightforward to fix in the validation logic. @sahrens, you originally wrote that system - do you have any ideas?

tl;dr; for colors and images, we're post-processing the value in the prop before passing to the underlying native view, with the result that the type no longer matches the public propType, causing a warning.

There's no way to specify that the native component has a prop of the same name _but different type_ than the JS wrapper. We can mark props as native-only, but not native-different.

We could rename those props on the native component, but that's horrible.

Maybe we should just change the validator to check nativeOnly first, then we can mark them as nativeOnly to ignore the public prop?

Or we could add a "nativeDifferent" key? That still seems unnecessarily manual though.

Do we actually get much value from this validation? If the types are wrong they'll be flagged on the native side anyway.

[sahrens]

Since the native code checks the type, maybe we could just kill the JS-side
type checking?

On Monday, December 7, 2015, Nick Lockwood [email protected] wrote:

This shpuld be straightforward to fix in the validation logic. @sahrens
https://github.com/sahrens, you originally wrote that system - do you
have any ideas?

tl;dr; for colors and images, we're post-processing the value in the prop
before passing to the underlying native view, with the result that the type
no longer matches the public propType, causing a warning.

There's no way to specify that the native component has a prop of the same
name _but different type_ than the JS wrapper. We can mark props as
native-only, but not native-different.

We could rename those props on the native component, but that's horrible.

Maybe we shpuld just change the validator to check nativeOnly first, then
we can mark them as nativeOnly to ignore the public prop?

Or we could add a "nativeDifferent" key? That still seems unnecessarily
manual though.

Do we actually get much value from this validation? If the types are wrong
they'll be flagged on the native side anyway.

—
Reply to this email directly or view it on GitHub
https://github.com/facebook/react-native/issues/4547#issuecomment-162650032
.

[nicklockwood]

Works for me.

[christopherdro]

So after going through each component the only props that are being post processed are tintColor and date.

tintColor appears across multiple components whereas date is just through DatePickerIOS.

I don't think its a bad idea to keep the prop types since it provides us a extra layer of validation before calling the native component and serves as documentation for each component.

Here are some of my thoughts on how to fix the two props.

date

We can add PropType.number to date, minimumDate & maximumDate, allowing a unix timestamp as a possible option.

I think its fair to have a date object or a timestamp as valid inputs. From here we can check the prop type and call getTime() if a date object is passed in.

tintColor

After taking a closer look there are already inconsistencies between the tintColor and its prop type across different component. If we use processColor we can have strings, arrays and objects as valid prop types so we should already go through and all component consistent.

This might be a bit of a stretch but if we added PropType.number as a option to tintColor allowing the bit value to be passed in, this would cover all aspects and get rid of the warnings.

Hopefully this makes sense and that I understood what you meant by killing the validation on the JS-side.

[nicholasalanbrown]

+1 for fixing this soon, I get Invalid PropTypes warnings for both the date and onDateChange props event when following the example for DatePickerIOS directly.

[ide]

Just so we're on the same page -- I believe there are 3 levels of validation:

1. JS validation of the public component, like ScrollView
2. JS validation of the bridged native component, like RCTScrollView (React component)
3. Native validation of the bridged native component, like RCTScrollView (Obj-C class)

and "killing the JS-side type checking" solely refers to step 2. Do I have that right?

It'd be nice to find a way to keep it to catch mistakes earlier than later, but aside from that it does seem somewhat redundant with step 3. But it does only matter when a prop's type changes from the public component to the native component (e.g. Date -> number), so this should affect only authors of bridged components.

[nicklockwood]

@ide correct. There's a bug in the native-side error logging at the moment which means you don't find out which prop has been set wrong, so we should probably fix that first before getting rid of the JS-side validation, however native-only properties should only be set in the JS wrapper for the component, so loss of the JS stack trace shouldn't be a big deal.

[bestander]

So @nicklockwood @ide what did we decide on in the case of colors Prop in PullToRefreshLayoutAndroid.android.js (step 2 from above)
Do we convert colors property to any in that file?

[nicklockwood]

I think we should just define ColorPropType as being any of the supported color formats.

[jblack10101]

+1 for the DatePickerIOS issue

[afilp]

Maybe this is also related to the same issue?
https://github.com/facebook/react-native/issues/5111

[nicklockwood]

@af7 AFAIK, we're now using ColorPropType everywhere, so this warning shouldn't be happening any more.

[christopherdro]

@af7 Here is that commit https://github.com/facebook/react-native/commit/a162f72655505e5da28e4033b2ef1d49cfcf067f

[marcshilling]

Warning: Failed propType: Invalid prop 'date' of type 'Number' supplied to 'RCTDatePicker', expected instance of 'Date'
...I am passing an instance of 'Date'

Warning: Failed propType: Required prop 'onDateChange' was not specified in 'RCTDatePicker'
...I am specifying that prop

[nicklockwood]

@marcshilling you can safely ignore this warning. It's because we convert to a number to send to the underlying component prop, which uses the same validation. We'll fix it at some point.

[christopherdro]

@nicklockwood Should I still keep this open?

[nicklockwood]

@christopherdro I guess so - it hasn't been fixed yet.

[christopherdro]

I guess i'll just leave it open until it gets fixed.

[ChangJoo-Park]

+1

[boyaq]

+1 for DatePickerIOS warning

[yimiaodaren]

+1

[mkonicek]

Guys if you want to get this visibility can you post it to Product Pains where people can vote and see what the most important issues are? People can login with GitHub so it's quite seamless:
https://productpains.com/product/react-native/?tab=top

[mtford90]

+1, rather annoying because I get spammed with the yellow warning boxes in every view that has a DatePickerIOS ;)

[alexprice1]

+1

[tronin]

+1
Required proponDateChangewas not specified inRCTDatePicker``

[slamus]

Guys,

Please know that you can make those box disappear by putting this in the root file of your app:

    console.ignoredYellowBox = [
      'Warning: Failed propType',
      // Other warnings you don't want like 'jsSchedulingOverhead',
    ];

See you !

[benceg]

@slamus works for me albeit as a temporary measure. Thanks for that!

[danginirav]

@slamus It works very nice. Thank you!
Instead of putting this in the root file, I prefer to put in the particular file (Component File) where the DatePickerIOS control exists.

[slamus]

@danginirav, I like that ! I will do that too.

[crazybear]

+1

[zires]

+1

[magrinj]

+1
If think the solution of @christopherdro is great, add possibility of putting a number in propTypes. I currently use a fork, I'm gonna do a PR.

[ianakiara]

+1

[CodeTitanian]

+1

[mkonicek]

GitHub has a better way to vote on issues now (under the issue description):

[edgesite]

Hello, I just fixed the issues with DatePickerIOS. https://github.com/facebook/react-native/pull/7472

[magrinj]

@rikuayanokozy I do something like you, but mine allow two kinds of props for date, Date and number to allow use of timestamps #6981
I do my PR base to the idea of @christopherdro

[digitalcatnip]

AFAIK, I could not find a ProductPains issue referencing an issue with datepicker or prop conversion. If there is another one let me know.

In the meantime, I raised it here:
https://productpains.com/post/react-native/fix-validation-on-changing-proptypes-js-native

Please increment it to help get the issue fixed. @magrinj 's fix is currently stuck in review.

[avioli]

The best solution is to wait for the RN team to implement a proper one.

Meanwhile ignoring certain messages in the yellow box would be the simplest temporary solution.
What I did was to set the DatePickerIOS' propTypes to something permissive:

// ./App/Components/DatePickerIOS.ios.js
import { PropTypes } from 'react'
import { DatePickerIOS } from 'react-native'

DatePickerIOS.propTypes.date = PropTypes.any.isRequired
DatePickerIOS.propTypes.onDateChange = PropTypes.func

export default DatePickerIOS

And then I imported that component.

[seigel]

@avioli Has a great solution for the short term to avoid the yellow boxen. :) Obviously for anyone that is using other properties you one can add their own additional proptypes for those properties as well. For instance:

...
DatePickerIOS.propTypes.maximumDate = PropTypes.any;
DatePickerIOS.propTypes.minimumDate = PropTypes.any;
...

[mhollweck]

+1 any updates?

[mkonicek]

Just read through this - it looks like we have an agreement what to do and just need to do it. Anyone up for sending a PR? :) The React Native team is pretty swamped as usual, we welcome contributions though.

Until someone sends a PR, please use the workarounds mentioned in the comments.

From comments above:

@sahrens:

Since the native code checks the type, maybe we could just kill the JS-side type checking?

@nicklockwood:

Works for me.

@ide:

Just so we're on the same page -- I believe there are 3 levels of validation:

JS validation of the public component, like ScrollView
JS validation of the bridged native component, like RCTScrollView (React component)
Native validation of the bridged native component, like RCTScrollView (Obj-C class)
and "killing the JS-side type checking" solely refers to step 2. Do I have that right?

@nicklockwood:

Correct.

...

I think we should just define ColorPropType as being any of the supported color formats.

[magrinj]

I've done one #8211

[javache]

Could anyone please provide an example of code that currently shows a warning, and is removed when using #8211?

[magrinj]

For me I got the warning when DatePickerIOS is mounted

[javache]

Please provide an example inside UIExplorer. As it is right now, I don't see any warnings from opening the DatePickerIOSExample in UIExplorer.

[javache]

I think this has already been fixed by https://github.com/facebook/react-native/pull/7833

[magrinj]

I didn't see that, the solution provide by #7833 is better!

[tangkunyin]

Thanks @avioli
after add this below,the problem fixed

import {PropTypes,DatePickerIOS } from 'react-native';

DatePickerIOS.propTypes.date = PropTypes.any.isRequired;
DatePickerIOS.propTypes.onDateChange = PropTypes.func;

dont not foget import PropTypes