-
Notifications
You must be signed in to change notification settings - Fork 8
Fix NestedRoutes 'redirectToIfNotFound' bug #29
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 NestedRoutes 'redirectToIfNotFound' bug #29
Conversation
…dual NestedRoute to fix fallthrough to <Redirect>, add test for this case
Codecov Report
@@ Coverage Diff @@
## master #29 +/- ##
===========================================
- Coverage 100.00% 93.65% -6.35%
===========================================
Files 9 12 +3
Lines 166 205 +39
Branches 18 23 +5
===========================================
+ Hits 166 192 +26
- Misses 0 12 +12
- Partials 0 1 +1
Continue to review full report at Codecov.
|
🤦 Looks like codecov is seeing a decrease in coverage, probably because this is the first set of tests that are hitting any of the routing components. I'll add some issues to backfill tests for these. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brandongregoryscott thank you for troubleshooting this issue! I'm going to accept, merge and build a release. Going to update the boilerplate to use this. Nice work!
As for test coverage, yeah, I see what you mean. No worries! This is an increase in coverage in my book 👍
const redirectToIfNotFound = notFoundRoute.path; // This is the important setup | ||
|
||
const App = () => ( | ||
<Router history={history}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought: Could be wrong, but think you could spare the extra setup of 'history' by using MemoryRouter
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah, I was seeing that in the docs too. Wasn't sure how I would simulate a navigation change, though it looks like MemoryRouter
has an initialEntries
prop which may have worked the same way.
@@ -0,0 +1,113 @@ | |||
import React from "react"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brandongregoryscott Thank you for writing tests for this!
Fixes #26
Wrap
NestedRoutes
render in<Switch>
and expand route as props to individualNestedRoute
to fix fall-through to<Redirect>
, add test for this case