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

Fix <Route component /> type #6327

Closed
wants to merge 3 commits into from
Closed

Fix <Route component /> type #6327

wants to merge 3 commits into from

Conversation

eXon
Copy link
Contributor

@eXon eXon commented Sep 10, 2018

The <Route component /> prop type at the moment is a function. However, if the component has a forwardRef

You might not have the control over this if using third-party HOC on your component. As an example, I use withCookies on my routed component. The result would be a forwardRef because the HOC make sure you still can use ref on your original component.

To fix this, we need a better prop-types. Unfortunately, making it reliable isn't as simple as it might look like. There is already a PR to add PropTypes.elementType to the official prop-types library. For now, there is a library with the same code (prop-types-elementtype).

I've added a failing test and added the better prop-types.

Let me know if I need to do anything else to get this merged!

@mjackson
Copy link
Member

Thanks for the PR, @eXon 😅 Can we just use a PropTypes.object check instead of importing a separate lib?

@frehner
Copy link
Contributor

frehner commented Oct 16, 2018

It looks like the tests are failing only because the history function isn't created in the it statements. Should hopefully be an easy fix. :)

Also, correct me if I'm wrong (which I probably am!) -- but if we're not spying on console.error, then the support forwardRef component test is going to pass even if the prop type isn't changed, since the expect statement will still be valid. Maybe want to add a spy on console.error or some way to ensure that the test is testing what it says it is?

@timdorr
Copy link
Member

timdorr commented Oct 16, 2018

@mjackson PropType.object is just any object. react-is (under the hood) will do a more comprehensive check for it's element-ness.

@mjackson
Copy link
Member

Ah, great point @timdorr. Ignore my comment, @eXon. Can you please update the PR so the tests pass? Thanks!

@frehner
Copy link
Contributor

frehner commented Oct 23, 2018

I created a new PR for this that addresses the feedback here, while also cleaning up some parts as well. #6417

@mjackson
Copy link
Member

Closing this in favor of #6417

@mjackson mjackson closed this Oct 25, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Dec 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants