-
Notifications
You must be signed in to change notification settings - Fork 980
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
Router.tsx: Test Route matching in Sets #2591
Conversation
packages/router/src/Set.tsx
Outdated
@@ -46,7 +46,7 @@ export function Set<WrapperProps>(props: SetProps<WrapperProps>) { | |||
...rest | |||
} = props | |||
const routerState = useRouterState() | |||
const location = useLocation() |
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 did a little research into this when I was writing up a ticket. When running a test to see if it did what was expected.
The location provider would get caught in an infinite loop due to this code
componentDidMount() {
this.HISTORY_LISTENER_ID = gHistory.listen(() => {
this.setState(() => ({ context: this.getContext() }))
})
}
So removing useLocation
should prevent that.
@Tobbe I think one thing that this PR doesn't cover is using just <Private />
I no we now recommned using <Set private />
but we didn't deprecate Private
so I think that this fix only fixes it for the Set
component
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.
This is the implementation of <Private>
export function Private<WrapperProps>(props: PrivateProps<WrapperProps>) {
const { children, unauthenticated, role, wrap, ...rest } = props
return (
<Set<any>
private
unauthenticated={unauthenticated}
role={role}
wrap={wrap}
{...rest}
>
{children}
</Set>
)
}
As you can see it uses <Set>
under the hood, so the fix covers <Private>
too
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.
Ahhh okay I didn't know it did that under the hood. Is it worth adding a test case for the Private
component just in case this changes in the future. Not sure it ever would But who knows what the future holds
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.
Thanks for this Tobbe. It looks good to me - just need to fix the linting errors.
995eb0d
to
f4bda61
Compare
31435c6
to
7956be2
Compare
If you tried to navigate to
/about
the private Set would see that/about
matches/{param}
(param
would be set to "about"), and if you weren't authenticated, you'd be redirected to the home pageThis PR fixes this by looking at the route name. It knows you're trying to go to the "about" page, so in the Set, it looks at name="param" and sees that "param" !== "about", and so it skips it.
Note
The actual fix for this was implemented by another PR a while ago. So just adding the specific test for this issue now.