support nested routes, and a base prop to start from if necessary

[sskoopa]

[developit]

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?

[sskoopa]

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.

[sskoopa]

Updated to use props.base, tests still pass

[developit]

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 };
	}
}

[sskoopa]

Once base is calculated, where do you store it to use in getMatchingChildren?

[developit]

Could just leave it as this.base - the reason to skip passing it into state was to avoid a re-render.

[sskoopa]

I'll work that direction then

[sskoopa]

s/this.base/this.baseUrl works, of course this.base is already a preact variable :)

[developit]

Ahhh haha I completely missed that

[stonecobra]

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.

[matannoam-zz]

+1

[developit]

this is high on my todo list, we're using workarounds right now and it's ugly.

[sskoopa]

Anything I can help with? Do you want to see something else changed? I am willing to help.

[developit]

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 :)

[osdevisnot]

+1. Planning to switch our app to preact-router once nested routes are in.

[developit]

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.

[sskoopa]

@developit I updated the PR to remove conflicts, ready to merge

[developit]

Awesome, thanks a bunch! Hopefully we can merge this, right now we are hacking around it haha.

[developit]

I tried and failed to resolve conflicts, didn't want to botch it.

[sskoopa]

@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?

[sskoopa]

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 :)

[marcodeltongo]

This would be super-useful to avoid workarounds, anything missing for approval?

[chemzqm]

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/

[eligao]

Hi, anything I could help with this? Would be nice to have it.

[jahilldev]

@sskoopa @developit Any news on this? Really don't want the extra size of React Router as this does everything I need. Thanks

[developit]

If anyone wants to take a stab at resolving the conflicts that'd help. We all want it merged :)

[stevengrimaldo]

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?

[matannoam-zz]

Hi quickly updated - https://github.com/developit/preact-router/pull/293

[hbroer]

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?

[marvinhagemeister]

@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 🎉