-
-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
Add exact and strict support to renderRoutes #4826
Conversation
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.
I think that we should be testing renderRoutes
directly, not just integration. What I would like to see tested would be two types of routes:
exact
exact
andstrict
You can test strict
without exact
, but you just need to make sure that you understand how that works (see https://reacttraining.com/react-router/core/api/Route/strict-bool). The <Route>
tests don't actually test this, just strict
and exact
.
For each one, there should be two cases: one where it should match and one where it should not. For example, this is the <Route exact>
test from the react-router
package: https://github.com/ReactTraining/react-router/blob/master/packages/react-router/modules/__tests__/Route-test.js#L240-L270
} | ||
] | ||
}, | ||
{ |
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.
I don't think that this second route is necessary.
I wrote tests for |
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.
Pretty much just requests for style changes. No need to keep the code compact; easy to read code is preferable. Otherwise, I think that it looks good.
rendered.push(route), | ||
renderRoutes(routes) | ||
) | ||
beforeEach(() => rendered = []) |
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.
Every other before/after each call uses this style.
beforeEach(() => {
rendered = []
})
component: Comp, | ||
}] | ||
|
||
renderToString(<StaticRouter location='/' context={{}}>{renderRoutes(routes)}</StaticRouter>) |
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.
No need to cram everything into one line.
renderToString(
<StaticRouter location='/' context={{}}>
{renderRoutes(routes)}
</StaticRouter>
)
expect(rendered.length).toEqual(1) | ||
expect(rendered[0]).toMatch(routeToMatch) | ||
}) | ||
describe('Switch usage', () => { |
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.
Leave a blank line before the describe
.
expect(rendered.length).toEqual(1) | ||
expect(rendered[0]).toMatch(routeToMatch) | ||
}) | ||
it('renders the first matched route in nested routes', () => { |
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.
Blank line here, too.
Should be good to go ! |
That's a lot of tests! LGTM too. Good work, @lith-light-g! |
Any idea when this PR will find its way into a release ... would really help me out with a server rendering example I'm writing? |
+1 for more info when this will be released. |
If anyone is interested I've forked and published as @amurdock/react-router-config until the next release. |
My attempt at writing tests for #4728