Closed jeysal closed 3 years ago
Hey good idea and thank you for sharing your ideas.
When I designed this lib I had a specific use case in mind where a route always corresponds with the hierarchical order of one component tree. But this is not the case in your example since you have one (sub)route that is shared by two different component trees (a tab is not a parent of modal). Seems pretty legit.
Regarding idea 2:
This avoids the repetition, but defining the tabs as parameters with a custom parser is not nice. This can also be changed to have the tabs always as params, but that doesn't make the parser situation nicer.
I'm not sure if this an option for you but you could also narrow the type of the regular stringParser
to have the tabs always as parameters as you said:
const edit = route("/edit", {}, {});
const tab = route('/:tab', {
tab: stringParser as Parser<"tab1" | "tab2" | "tab3">,
}, { edit });
This reduces the number of required Route
components to only two.
<Route path={tab.template}>...</Route> // tab tree
<Route path={tab.template + edit.template}>...</Route> // modal tree
However there is no type safe method to chain route templates yet. In my projects I usually use simple string concatenation to combine multiple paths similar to the seconds route in the example above.
A few possible solutions for that:
combineRoutes(tab, edit).template
has a nice api but I'm afraid the implementation will be a pain in the ass because the hierarchy of the routes has to be taken into account, too. tab.edit.template
would be prone to conflicts (e.g. /template
)tab.children.edit.template
would be to verbose for deeper routesLet me know you thoughts.
I'm not sure if this an option for you but you could also narrow the type of the regular
stringParser
to have the tabs always as parameters as you said:
That's about what I thought, except I thought you would also write actual custom code rather than just casting the string parser, to make it throw when a string is given that is not a valid tab. But maybe I misunderstand how parsers work, haven't tried custom parsers yet.
Either way, I wouldn't consider option 2 ideal. I consider params to be for values in the path that you don't know at compile time with "infinite" possibilities, while the normal nice root({}).abc({})
api is used where there is a fixed set of knows child routes. Thinking about it, there's really nothing special about 'tabs' either - it could equally be multiple pages that can all have the modal stacked on top of them, and you wouldn't express sub-pages as root({}).child({page: 'abc'})
.
That's why I tried to find a solution where the route structure is defined normally as in options 1 and 3, but we don't have the repetition of 1 or the hack of 3.
Regarding concatenation, a helper for that may also be nice but I think that's actually a different problem, I would be fine still doing Route path={path + combineTemplates(tab1, tab2, tab3)}
, so still doing the concatenation myself.
As for how to implement this combineTemplates
I gave it a thought and it seems it should be possible because I think react-router
via path-to-regexp
supports paths like /tabs/:tab(tab1|tab2|tab3)
, so that combineTemplates
could generate the /:tab(tab1|tab2|tab3)
bit. The name of the parameter should probably be randomly generated because it is not used (just there to match all three combined paths) and should never conflict with any real parameter.
But I'm not sure this is in scope of this library anymore actually, because with the format of the path it generates it's somewhat specific to react-router
\ path-to-regexp
.
That's about what I thought, except I thought you would also write actual custom code rather than just casting the string parser, to make it throw when a string is given that is not a valid tab. But maybe I misunderstand how parsers work, haven't tried custom parsers yet.
I don't think you misunderstand how custom parsers work. For your specific case it depends on the type of the identifier tab(1|2|3)
. Intuitively I would validate a parameter as soon as possible e.g.:
const TABS = ["tab1", "tab2", "tab3"] as const;
type Tab = typeof TABS[number];
const tabParser: Parser<Tab> = {
parse: (p: string) => {
if(!TABS.includes(p)) {
throw ...
}
return p as Tab;
}
...
}
However if tab(1|2|3)
is an identifier of a dynamic resource that you need to fetch from a back end you wouldn't validate its' value in a parser since parsers are not supposed to fetch anything, right? So if you used parameters only for addressing dynamic resources (restrict them unknown values at compile time) then a parser validation would always be limited to the format of its' values (e.g. base64, iso date, etc...).
Yeah in my case, they are well-known at compile time, otherwise I'd use a route param
Maybe some sort of
path={combineRoutes(tab1, tab2, tab3).template}
that generates a route matching/tab1
and/tab2
and/tab3
?
path-to-regexp
supports paths like/tabs/:tab(tab1|tab2|tab3)
, so thatcombineTemplates
could generate the/:tab(tab1|tab2|tab3)
bit.But I'm not sure this is in scope of this library anymore actually, because with the format of the path it generates it's somewhat specific to
react-router
\path-to-regexp
.
path-to-regexp
is pretty widespread module so I wouldn't mind to utilize more of its' features for typesafe-routes. However before considering this I would suggest an alternative approach. This module already exports a few react-router components, why no adding one more to cover this one use case?
If I understand correctly the main purpose of this issue is finding a method to map multiple routes to a singly component without violating DRY principle, right? So what about a react component that creates a set of react routes with:
<CombinedRoutes to={[tab1.edit, tab2.edit, tab3.edit]}>...</CombinedRoutes>
that would dynamically generate something similar to this:
<Route path={tab1.edit.template}>{p.children}</Route>
<Route path={tab2.edit.template}>{p.children}</Route>
<Route path={tab3.edit.template}>{p.children}</Route>
Could that be a suitable solution for now?
The alternative combineRoutes
function that generates /:tab(tab1|tab2|tab3)
would be an option, too. Though probably difficult to implement (maybe you know a easy way to group routes and allow usage of common children?). But for now we only have an use case in react that simplifies things a little. So there is probably no need for premature generalization (if I'm not completely wrong).
Btw. thank you for your patience. :pray: I noticed that you are always answering almost immediately to my posts. :pray:
@kruschid thanks, yeah that actually seems very sensible, I should try that. I was probably thinking way too much in a "I'm introducing this library and it must fit one to one into our existing usage of React Router" way :sweat_smile:
Well I the actual idea was to include the CombinedRoutes
component into the typesafe-routes
npm package. As I mentioned we are already shipping some optional react specific utilities/components with the npm release (useRouteParams
, Link
, NavLink
) and I was thinking about adding a Route
wrapper component anyway. So I could add CombinedRoutes
to the same release.
turns out CombinedRoutes
is actually already a thing and called Route
, which accepts an array for the path
prop :joy: so yeah I guess this is solved ^^
This is more of a discussion issue at this point, I just encountered something that was a bit awkward to express using this library and react-router and thought maybe we could brainstorm whether there is a pretty solution to this. I'm not actually sure there is something the library can do to help with this, but maybe I'm missing something and someone else has an idea.
The problem
We have a page with three tabs. So far straightforward. However, over each of these tabs, the same semi-opaque modal for editing some information in the page header can be rendered. The route structure is the following:
With typesafe-routes, what would the structure be?
Options so far
This allows us to have the edit route as a child of each tab route, but the repitition is of course a bit meh :sweat_smile:
This avoids the repetition, but defining the tabs as parameters with a custom parser is not nice. This can also be changed to have the tabs always as params, but that doesn't make the parser situation nicer.
This feels similar to 1, but it's nice that we can also use
tabRoute
to dynamically build an href to theedit
route of the current tab:<Header editHref={tabRoute({}).edit({}).$} />
Solutions
This is where I'm not sure. Maybe some sort of
path={combineRoutes(tab1, tab2, tab3).template}
that generates a route matching/tab1
and/tab2
and/tab3
?