Support for parent prop in Router

[molefrog]

It's an alternative solution to #231.
Notable differences:

1. Because <Router /> is immutable (it only gets created once and doesn't react on props change) we can simply make a copy of the parent router, it does not have to be dynamic.
2. Changed the API, there is no need now to write parent={useRouter()} which IMO could be not obvious for some users. Instead, by adding a nested prop, we the the router to inherit the base path. I even think that we can enable this by default in future releases.
3. Simplified the context it passed an object instead of a reference. This might sound like a performance downgrade, but in reality because Router is immutable it never changes between renders. Hence, context update won't be triggered.

[github-actions]

size-limit report 📦

Path	Size
index.js	1.11 KB (+0.36% 🔺)
use-location.js	200 B (+25.79% 🔺)

[codecov]

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (03fe1e7) compared to base (2a4b522).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #265   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines          255       262    +7     
  Branches        50        50           
=========================================
+ Hits           255       262    +7     

Impacted Files	Coverage Δ
preact/react-deps.js	100.00% <ø> (ø)
index.js	100.00% <100.00%> (ø)
react-deps.js	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[HansBrende]

This change is ok... however... it will lead to inefficiencies down the road. Because you are going to want to change <Route> at some point to support nested routes (you can calculate base automatically from the path pattern!!!)... in which case, you will already have the parent router and can supply it directly to the child <Router> without having to call useContext() a second time redundantly. In addition, for non-nested routers, you are now adding an extra useContext() call for no reason.

My recommendation would be to simply supply parent instead of a nested flag. This makes the routers more powerful and maximally efficient.

People want the nested router ability so that they can wrap your library with their own code for nested Routes. So, it is more much important to make Routers powerful than marginally more concise, since actual usage would be at the Route level, not the Router level. A more powerful Router is merely the first step in building more powerful Routes. Ideally, users shouldn't have to use Routers at all.

But once nested Routers are working, you would then be free to turn your focus to allowing users to not have to use Routers at all, by instead allowing a nested Route to work and auto-calculate the base for the nested child router beneath them. ("nested" would be a good flag for Routes instead of Routers!)

[molefrog]

This change is ok... however... it will lead to inefficiencies down the road. Because you are going to want to change <Route> at some point to support nested routes (you can calculate base automatically from the path regexp!!!)... in which case, you will already have the parent router and can supply it directly to the nested <Router> without having to call useContext() a second time redundantly. In addition, for non-nested routers, you are now adding an extra hook call for no reason.

My recommendation would be to supply parent instead of a nested flag. This makes the routers more powerful. And then, turn your focus to allowing users to not have to use Routers at all, by instead allowing nested Route to work and auto-calculate the base for the nested router beneath them. ("nested" would be a good flag for Routes instead of Routers!)

Not sure if I follow, could you please provide some example on how router and nested routes could be used together?

[HansBrende]

@molefrog Sure, I will put some examples together from my own codebase. Will get back to you soon.

[HansBrende]

@molefrog this is (roughly) how my own codebase defines Route:

function splitPathIntoBaseAndRest(pattern: Path, path: Path, matchParams: DefaultParams) {
    const matchKey = /:([^/:]+)\*$/.exec(pattern);
    if (!matchKey) return null;
    const rest = matchParams[matchKey[1]];
    if (rest == null || !path.endsWith("/" + rest)) return null;
    return [path.substring(0, path.length - rest.length - 1), "/" + rest];
}

export function Route<T extends DefaultParams | undefined = undefined, RoutePath extends Path = Path>(props: RouteProps<T, RoutePath>) {
    const {path, children, component: Component} = props;

    const parentRouter = useRouter();
    const [pathToMatch] = parentRouter.hook(parentRouter); // note: this is not the default hook

    const [matches, params] = (props as any).match as Match ?? parentRouter.matcher(path ?? '', pathToMatch);
    if (!matches) {
        return null;
    }
    const [base, rest] = splitPathIntoBaseAndRest(path ?? '', pathToMatch, params!) ?? [pathToMatch, ''];
    // Example of above function:
    // Input: pathToMatch "/posts/1234/comments/5678" split using pattern "posts/:postId/:_*"
    // Output: base = "/posts/1234"; rest = "/comments/5678"
    
    // THE IMPORTANT TAKEAWAY:
    // I already needed to use the parent router to perform my calculation,
    // so now I can just pass it in below as a prop, extra useContext() call is unnecessary!!
    //     ↓           ↓
    return <Router parent={parentRouter} base={base} hook={myCustomHookThatReturnsRest}> 
        // actual children go here (omitting for brevity, but they are a function of `rest`)
    </Router>;
}

As you can see... in performing the calculation to allow nested Routes, I already have access to the parent router so it seems silly to require an extra useContext call when I can just pass it in as a prop.

To illustrate how this actually works in jsx:

<Route path='/posts/:postId'>
    <Route path='comments/:commentId'> // matches "/posts/1234/comments/5678"
        ...
    </Route
</Route>

If you want to do something like this as well in the future (which seems very likely since this is incredibly powerful), then you'll probably not want to add superfluous useContext calls either. If you allow parent={useRouter()} syntax, then you future-proof yourself in this respect. (While also avoiding unnecessary overhead for non-nested routers).

[molefrog]

Thanks, I'll review the code and get back to you tomorrow.

[molefrog]

Alright, I think I finally get it.

("nested" would be a good flag for Routes instead of Routers!)

That's the most important note here! For some reason I thought that maybe we could use nested routers as a replacement for nested routes, but now it makes more sense to have a semi-private prop (for advanced usage) that'll help us to build nested route functionality and much more.

[molefrog]

See 5963014

[HansBrende]

Looks awesome! 🎉

[molefrog]

Great! Thank you for your help @HansBrende.
I was thinking that we can add support for useSyncExternalStore next and include it in the same release. Would you want to work on that?

[HansBrende]

@molefrog sure thing, I can work on that.

[HansBrende]

@molefrog PR for useSyncExternalStore submitted! #267

[molefrog]

@HansBrende FYI I've decided to release it, the change has landed in 2.9.0.