search

acdlite / redux-router

Redux bindings for React Router – keep your router state inside your Redux store
MIT License
2.3k stars 200 forks source link

Tests on Invalid prop `RoutingContext` supplied to `ReduxRouterContext`. #266 #267

Closed catamphetamine closed 8 years ago

Scarysize commented 8 years ago

Cool, thank you! I hope, I will come around to get this fixed. Though I have no clue why this fails yet.

catamphetamine commented 8 years ago

@Scarysize Ok, then I updated the Pull Request to include the fix as well. The clue why it failed before is in the test code: PropType.extends(Component) (if it existed) and PropType.instanceOf(Component) are different things.

Scarysize commented 8 years ago

Nice. Should we add an additional test for stateless components?

catamphetamine commented 8 years ago

@Scarysize Added a test for a stateless component

Scarysize commented 8 years ago

@halt-hammerzeit Just some minor issues. Fix these and I'm happy to merge. 👌

catamphetamine commented 8 years ago

@Scarysize did some splitting and variable extraction

Scarysize commented 8 years ago

Nice work 👍

Scarysize commented 8 years ago

If @mjrussell has given it a quick overlook I will merge this.

mjrussell commented 8 years ago

I saw the original issue, but Im not sure why RoutingContext isn't allowed to be a Component or a function. Is there a reason why we are explicitly preventing people from doing this?

catamphetamine commented 8 years ago

@mjrussell

not sure why RoutingContext isn't allowed to be a Component or a function

It wasn't allowed to be a Component or a function, and it is allowed that in this PR. Just to clarify things.

mjrussell commented 8 years ago

Sorry I should have been more clear. It looks like you are restricting it to just functions, shouldn't the proptype be 

PropTypes.oneOfType[PropTypes.func, PropTypes.element].isRequired

Or is there currently no warning if you pass a React class (non instantiated like your test).

catamphetamine commented 8 years ago

@mjrussell There's no PropType for React components. The code you wrote is invalid.

mjrussell commented 8 years ago

Was on my phone so I missed a paren.

PropTypes.oneOfType([PropTypes.func, PropTypes.element]).isRequired

Is definitely valid. Check out the docs - https://facebook.github.io/react/docs/reusable-components.html

It might not be necessary to add the element option but I'll write a quick test

catamphetamine commented 8 years ago

@mjrussell It is valid syntactically but is wrong conceptually. Refer to React docs to read more about what is an "Element".

mjrussell commented 8 years ago

Did a sanity check with both React.createClass and extending React.Component. Looks good, I forgot that the element is for rendered instances, not react component definitions.

This looks good to me, I spotted a minor test typo I'll point out in the code

Scarysize commented 8 years ago

@mjrussell Thanks for the feeback!

Scarysize commented 8 years ago

@halt-hammerzeit The warning which will be printed during tests: 

Warning: Failed propType: Invalid prop `RoutingContext` of type `object` supplied to `ReduxRouterContext`, expected `function`. Check the render method of `Connect(ReduxRouterContext)`.

Comes from the should not accept non-React-components for "RoutingContext" prop of ReduxRouter test, am I right?

catamphetamine commented 8 years ago

@Scarysize Yes

Scarysize commented 8 years ago

Okay, just needed to be sure.