preact-router
preact-router copied to clipboard
support nested routes, and a base prop to start from if necessary
This is awesome @sskoopa! Only thought I had was maybe use props.base
directly and skip persisting it into state.baseUrl
. Think that'd work?
I was just going on the fact that base is a calculated field, and props are supposed to be immutable in react-like codebases, no?
We need to calculate it before setting the context in getChildContext(), as that method is not passed the context.
Updated to use props.base, tests still pass
Ack! Sorry, I hadn't meant to mutate this.props
heh. I figured something like:
class Router extends Component {
constructor(props, context) {
super(props);
let base = props.base || '';
if (props.history) {
customHistory = props.history;
}
if (context && context[CONTEXT_KEY]) {
base = context[CONTEXT_KEY] + base;
}
this.state = {
url: this.props.url || getCurrentUrl()
};
}
getChildContext() {
let base = this.props.base || '';
if (context && context[CONTEXT_KEY]) {
base = context[CONTEXT_KEY] + base;
}
return { [CONTEXT_KEY]: base };
}
}
Once base is calculated, where do you store it to use in getMatchingChildren?
Could just leave it as this.base
- the reason to skip passing it into state was to avoid a re-render.
I'll work that direction then
s/this.base/this.baseUrl works, of course this.base is already a preact variable :)
Ahhh haha I completely missed that
ok, I now:
- have a <Match> component that can pass context deeper if needed
- Router works with path attribute now, so will work just inside a Router
- updated DOM tests to check nested support for default, path, and <Match/>
- context baseUrl now handles variable match paths as well
- updated README example
Let me know if you think anything else should be there.
+1
this is high on my todo list, we're using workarounds right now and it's ugly.
Anything I can help with? Do you want to see something else changed? I am willing to help.
Nah it's more just me wrapping my head around things. I just need to play around with the nesting - you already have good test coverage here, that's what I usually ask people to do and you already did it :)
+1. Planning to switch our app to preact-router once nested routes are in.
Yes - sorry for dropping this on the floor, we're super keen to get this merged too since our apps are using a workaround right now.
@developit I updated the PR to remove conflicts, ready to merge
Awesome, thanks a bunch! Hopefully we can merge this, right now we are hacking around it haha.
I tried and failed to resolve conflicts, didn't want to botch it.
@developit, I merged everything the way I think it should, but one of the tests I wrote for Match is failing:
FAILED TESTS:
dom
Router
✖ should support nested routers with Match
Chrome 57.0.2987 (Mac OS X 10.12.4)
TypeError: props.children[0] is not a function
at Match.render (test/dom.js:1608:49)
at renderComponent (test/dom.js:978:39)
at renderComponent (test/dom.js:990:26)
at Router.forceUpdate (test/dom.js:1100:14)
at Router.routeTo (test/dom.js:1337:9)
at routeTo (test/dom.js:1204:19)
at route (test/dom.js:1189:10)
at Context.<anonymous> (test/dom.js:458:20)
I left the console.log of children in Match.render as the child is a vNode, and usually the Router did a cloneElement on match. Do we need cloneElement on Match.render()? If so, do we need more or other tests on Match itself?
OK, Match components are merged and tests pass. I think more people will use the simple Match, but the power is there. gzip size 2050 (2K+2). I could shorten the CONTEXT_KEY constant name :)
This would be super-useful to avoid workarounds, anything missing for approval?
Any update on this? My boss want to use sub directory according to this article https://fly.io/articles/one-hostname-to-rule-them-all/
Hi, anything I could help with this? Would be nice to have it.
@sskoopa @developit Any news on this? Really don't want the extra size of React Router as this does everything I need. Thanks
If anyone wants to take a stab at resolving the conflicts that'd help. We all want it merged :)
When I go to review the PR i'm not seeing any conflicts, is there a certain view I should be looking at?
I'm also very interested in adding the ability for nested routes, how can i help @developit?
Hi quickly updated - https://github.com/developit/preact-router/pull/293
This would be so nice to see it merged. Big thumbs up to @sskoopa. @developit Is there any change to see this merged? Can i do something like local merge and manual testing maybe?
@hbroer Agree the PR is really great and in my opinion there is not much standing in the way of a merge. The PR needs to be updated against the latest changes in master for Preact X, but that seems to be all that's left to do 🎉