[Router] improve comments and consolidate validation logic #3977
Conversation
| useAuth, | ||
| paramTypes, | ||
| pageLoadingDelay, | ||
| trailingSlashes = 'never', | ||
| children, | ||
| }) => ( | ||
| }: React.PropsWithChildren<RouterProps>) => ( |
There was a problem hiding this comment.
I've seen React.FC discouraged a few times, but don't understand the argument enough to have my own opinion:
There was a problem hiding this comment.
I'm not as consistent in my usage as I should be. At work we always use React.FC or React.VFC (mostly VFC because most of our components don't take children, appart from our UI lib components). Redwood don't use FC/VFC. So I'm influenced by both and end up mixing :/
|
@Tobbe the |
jtoar
changed the title
|
|
||
| // Check for issues with the path. | ||
| validatePath(path) | ||
| path as string |
There was a problem hiding this comment.
I added this and other assertions to illustrate what I expect. At this point, path shouldn't be optional, since if a route isn't the not-found route, it has to either be a route or a redirect route (path is required for both).
There was a problem hiding this comment.
Having a type assertion on a line of its own like that, does it actually do anything?
| .filter((child) => isRoute(child) && !child.props.notfound) | ||
| .forEach((child) => { | ||
| const { path, redirect, page, name } = ( | ||
| child as React.Component<RouteProps & RedirectRouteProps> |
There was a problem hiding this comment.
Another type assertion to illustrate what should only be in the array at this point.
I didn't know about isValidElement. Thanks for showing me :) Should totally use that instead! |
| } | ||
|
|
||
| /** | ||
| * Note that this technically makes the router impure. |
There was a problem hiding this comment.
Now that you've pointed out that we're deviating from best-practice I kind of feel like your comment needs to include some kind of justification for our choice.
There was a problem hiding this comment.
Just added some more copy
There was a problem hiding this comment.
Looking forward to reading it when you're ready to push it 🙂
|
Totally subjective, but I'd prefer to keep Any reason you prefer Generally my rule-of-thumb is to use |
Co-authored-by: Tobbe Lundberg <[email protected]>
Co-authored-by: Tobbe Lundberg <[email protected]>
| * ``` | ||
| */ | ||
| flatChildArray | ||
| .filter((child) => isRoute(child) && !child.props.notfound) |
There was a problem hiding this comment.
See if you can include something in the big comment above about why we're skipping notfound here
|
We'll take a closer look at the router soon after React 18 goes in and we'll address these kinds of thing then. |
|
Lot's of good stuff in this PR. Definitely think we should revisit once #4992 has landed |
As a follow-up to #3772 and to keep the work on the router going, I wanted to do some of the some of the easier refactors before I forgot how this thing works again. Namely by
peerDependenciesMost of the changes are just comments. Even in
router.tsx, although that's where I strived to consolidate the validation logic so a little more's going on there.@Tobbe I've removed the custom
isReactElementfunction in favor ofReact.isValidElement, which according to the React docs, does what the former sought to do. Was there any reason to prefer it?I want to do more, but didn't want this to consume me. This is how far I got.