SteffeyDev / react-native-popover-view

A well-tested, adaptable, lightweight <Popover> component for react-native
MIT License
613 stars 92 forks source link

Remove propTypes #86

Closed ugrky closed 4 years ago

ugrky commented 4 years ago

Is your feature request related to a problem? Please describe. I am currently trying to run the library on react-native-web. I know that the library is not officially supported on web, but would be nice to make it work. I will be doing some changes on my own fork, but wanted to use this thread as a platform to discuss a few things.

react-native-web has unspoorted ViewPropTypes as of 0.12. Therefore, I get the following error:

Popover.tsx?f681:1 Uncaught TypeError: Cannot read property 'style' of undefined

Caused by the following lines:

popoverStyle: ViewPropTypes.style,
arrowStyle: ViewPropTypes.style,
backgroundStyle: ViewPropTypes.style,

Describe the solution you'd like First I want to ask you why is PropTypes used in this repo while there is already great TypeScript implementation. If we get rid of PropTypes, it will work on react-native-web too. I will work on further improvements to make this library work on web, but I think it would be the first step.

I would be happy if you could inform me why you choose to opt in for PropTypes.

Thanks in advance!

SteffeyDev commented 4 years ago

Great question. For people who use typescript, then the types are great. However, any developers who use this library in a non-typescript project need PropTypes to ensure that React stops them from passing in invalid props. I'm not sure how libraries that support web handle this use case, maybe you could see how they do type enforcement for non-typescript projects and let me know?

nandorojo commented 4 years ago

@SteffeyDev asked me to weigh in here. I only use TypeScript, so I’m not sure, but this might be relevant to https://github.com/necolas/react-native-web/issues/1684

nandorojo commented 4 years ago

Just updated my comment with the correct link.

SteffeyDev commented 4 years ago

Not sure if I missed it, does that issue relate to PropTypes at all?

nandorojo commented 4 years ago

Oh I see, this only relates to proptypes, not flow types, sorry got them mixed up since I don’t use either.

nandorojo commented 4 years ago

@SteffeyDev I ran into the same problem as this issue with react-native-calendars. I figured out a solution there.

All you have to do is conditionally import ViewPropTypes from react-native, and not import it on web. RNW removed it from the package since React Native plans on deprecating it.

This was the solution in react-native-calendars:

import { View, Platform } from 'react-native'

const viewPropTypes =
  typeof document !== 'undefined' || Platform.OS === 'web'
    ? PropTypes.shape({ style: PropTypes.object })
    : require('react-native').ViewPropTypes || View.propTypes
SteffeyDev commented 4 years ago

Ah very cool, I'll add this in and see if it works

nandorojo commented 4 years ago

To please TS, you might need to make the web type as any.

SteffeyDev commented 4 years ago

Actually TS has a problem with document and the fact that View does not have a member propTypes. I think I can exclude the document !== 'undefined' check, where did you get View.propTypes from?

nandorojo commented 4 years ago

Wix included that in their calendar library. I've never used prop types so I just copied that part over.

SteffeyDev commented 4 years ago

Interesting. Don't think I need that either.

SteffeyDev commented 4 years ago

Ok, this is done in the latest release, thanks for finding a good solution!