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

v4 does not include locationShape or routerShape #4419

Closed
billyjanitsch opened this issue Jan 31, 2017 · 4 comments
Closed

v4 does not include locationShape or routerShape #4419

billyjanitsch opened this issue Jan 31, 2017 · 4 comments

Comments

@billyjanitsch
Copy link
Contributor

Hi,

I noticed that the v4 API omits the locationShape and routerShape prop types provided in v3. Are there plans to add these back before the final release, or was their removal intentional?

If the latter, what's the recommended way of validating router props, e.g. when using withRouter, or for components passed to <Route>s? I understand the desire for a lighter API, but I also prefer the more thorough validation as opposed to a blanket PropTypes.object to make sure I'm getting the type of object I expect.

Thanks, and I appreciate all the hard work put into v4!

@mjackson
Copy link
Member

Just use whatever propTypes you need. If your component needs the listen method, for example, just use a React.PropTypes.func. It's more descriptive of what you actually need (you probably don't need all of the router's props) and it'll make for a smaller build.

@billyjanitsch
Copy link
Contributor Author

Gotcha -- I agree that's it's descriptive, though I'd also argue that it's more fragile.

// in one place:
location: PropTypes.shape({
  hash: PropTypes.string.isRequired,
  search: PropTypes.string.isRequired,
}).isRequired,

// in another:
location: PropTypes.shape({
  hash: PropTypes.string.isRequired,
  pathname: PropTypes.string.isRequired,
}).isRequired,

IMO, code like the above repeated throughout a codebase leaves more room for error and is quite verbose; I'd be tempted to write my own locationShape wrapper for reuse/conciseness. I also suspect the duplication would actually yield a larger build size (and a slightly slower startup, due to initializing many identical validators instead of a single shared one).

No need to reply if you still disagree; appreciate the consideration regardless. 🙂

@kutagh
Copy link
Contributor

kutagh commented Feb 1, 2017

How is it more fragile? If the location shape changes in an unexpected way, you will notice in development mode due to the PropTypes warning. This would make updating actually be more safe, as you now know when something breaks due to the change, where the actual location differs from the expected location.

@billyjanitsch
Copy link
Contributor Author

billyjanitsch commented Feb 1, 2017

@kutagh take another look at my second paragraph -- I was talking about fragility wrt potential for developer mistakes (due to duplication and frequent modification), not wrt updating dependencies.

In terms of the latter, I understand your point, but so long as react-router follows semver, locationShape should never change in a backwards-incompatible way for non-major releases.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants