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

Update exported PropTypes #3205

Closed
taion opened this issue Mar 18, 2016 · 12 comments
Closed

Update exported PropTypes #3205

taion opened this issue Mar 18, 2016 · 12 comments
Labels

Comments

@taion
Copy link
Contributor

taion commented Mar 18, 2016

I'm not convinced that we get anything out of exporting PropTypes. They're at least documented now, thanks to @lPadier's #3203, but I think we ought to consider dropping them.

With the possible exception of location, which is a matter of history rather than here anyway, they're not really applicable to users of this library, and things like history are even deprecated.

Analogous to how react-intl handles it, we should export routerShape as a top-level export. The other exports like location, to the extent they are required, should sit in history.

@agundermann
Copy link
Contributor

👍 In my opinion, importing prop types from other modules seems counter-productive. If a component uses location.pathname, for example, the component should be explicit about it (location: shape with pathname: string). By keeping prop types local, the component is more resistant to outside changes, e.g. if the location shape changed, the validation would fail, which it wouldn't with imported prop types.

In cases where a component only passes things along to other components, it should be sufficient to define it as an object or "any".

I wonder how common this is among react libraries.

@taion
Copy link
Contributor Author

taion commented Mar 19, 2016

I've never seen any other React libraries that export propTypes. I've never even heard of anybody using the exported propTypes here until #3203.

@ryanflorence
Copy link
Member

not sure how things made it into that list, but original intention was people to be able to add a contextTypes: { router: routerPropType } for their components that need the router.

@taion
Copy link
Contributor Author

taion commented Mar 20, 2016

That would make sense, but we've already extensively documented router: React.PropTypes.object, and I feel like that's adequate; enough that we don't need exported prop types.

@taion
Copy link
Contributor Author

taion commented Mar 21, 2016

@rickharrison Care to elaborate? 😛

@rickharrison
Copy link

I do use some of them personally. My thought was why take them out if they are already there considering it's so small. But, I guess if you don't want to maintain them I see why you would. I could also easily just include a similar file like that in my own project, but I'd have to duplicate it across our many react-router based apps.

@timdorr
Copy link
Member

timdorr commented Mar 21, 2016

IMHO I think most people are better served by a real typing system, such as TypeScript or Flow. PropTypes' duck typing never seemed that helpful to me, personally.

Looks like we need to update our PropTypes anyways. Given they're out of date, I would say we get rid of them.

@taion
Copy link
Contributor Author

taion commented Mar 21, 2016

Right, IMO the main thing is that they've just gotten quite out-of-date.

Plus the types for things like locations are owned by history anyway.

@ryanflorence
Copy link
Member

PropTypes are super valuable, but I'm not sure we need to export any of them except router

@taion
Copy link
Contributor Author

taion commented Mar 22, 2016

We're not currently exporting a router prop type, though.

@taion
Copy link
Contributor Author

taion commented Mar 22, 2016

I just found a counterexample. react-intl exports a shape. As I'm currently using react-intl, I am selfishly declaring for the sake of consistency that we should do the same in React Router.

I'm now in favor of continuing to export prop types, though perhaps with a slightly different API.

@taion taion changed the title Drop exported PropTypes? Update exported PropTypes Mar 28, 2016
@taion
Copy link
Contributor Author

taion commented Mar 28, 2016

Updated OP with my current thinking here.

@timdorr timdorr closed this as completed Apr 12, 2016
@lock lock bot locked as resolved and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

5 participants