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

sskoopa · 2016-12-11T18:15:51Z

No description provided.

developit · 2016-12-11T22:28:28Z

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 · 2016-12-12T02:47:11Z

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 · 2016-12-12T02:57:33Z

Updated to use props.base, tests still pass

developit · 2016-12-12T16:31:40Z

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 · 2016-12-12T16:53:03Z

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

developit · 2016-12-12T17:01:11Z

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

sskoopa · 2016-12-12T19:14:08Z

I'll work that direction then

sskoopa · 2016-12-12T19:22:59Z

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

developit · 2016-12-12T20:12:55Z

Ahhh haha I completely missed that

developit · 2016-12-15T16:32:03Z

src/index.js


 		this.state = {
 			url: this.props.url || getCurrentUrl()
 		};
 	}

+	getChildContext() {
+		return {CONTEXT_KEY: this.baseUrl};


This needs to be [CONTEXT_KEY].

stonecobra · 2016-12-19T01:17:05Z

ok, I now:

have a 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
context baseUrl now handles variable match paths as well
updated README example

Let me know if you think anything else should be there.

developit · 2016-12-19T01:40:57Z

src/index.js

 		super(props);
+		this.baseUrl = this.props.base || '';
+		if (props.path) {


I wonder if we need to move this into the if block on L156?

ghost · 2017-01-30T13:38:17Z

+1

developit · 2017-01-30T17:28:20Z

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

osdevisnot · 2017-02-17T03:23:02Z

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

developit · 2017-02-17T03:39:39Z

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 · 2017-03-04T17:12:27Z

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

developit · 2017-03-05T03:35:53Z

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

developit · 2017-04-17T23:45:18Z

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

sskoopa · 2017-04-18T03:42:43Z

@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 · 2017-04-18T03:46:12Z

test/dom.js

+				<Router base='/ccc'>
+					<X path="/jjj" />
+					<Match path="/xxx/:bar*">
+						<Router>


While the other nested router tests pass, the Match here cannot render the child as it is a vNode.

hmmm. yeah we might need to do type detection in Match. I realize now we sortof have two matches, so it might be good to merge them. Need to think on this for sure, because I like the options your Match gives us.

sskoopa · 2017-04-24T00:16:54Z

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 · 2017-07-29T15:34:24Z

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

chemzqm · 2017-08-18T12:18:15Z

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 · 2017-09-04T03:28:06Z

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

jahilldev · 2018-05-09T14:16:39Z

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

developit · 2018-07-31T17:25:30Z

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

stevengrimaldo · 2018-10-21T19:36:24Z

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?

ghost · 2018-11-09T01:53:54Z

Hi quickly updated - #293

hbroer · 2019-03-13T09:28:01Z

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 · 2019-03-13T09:50:53Z

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

Haugen · 2020-09-15T15:28:08Z

What happened to this feature? I can't seam to find that it was solved and merged in another issue. Is this still being considered or have it been discarded?

I would be happy to try and help solve the merge conflicts, but I'm not sure about the proper way to do it. Can I even update this PR?

developit · 2020-10-24T20:14:13Z

@Haugen it's still being considered, just hasn't been prioritized. There are so many great routers available that work with Preact, but the drawback is that it's hard to justify spending time on new features for preact-router. FWIW I would really like to get this functionality integrated, I just can't take that time away from current projects.

FWIW, @38elements recently contributed a guide section in the README that covers how nested Routers work today (without the changes in this PR): https://github.com/preactjs/preact-router#nested-routers

renanbandeira · 2021-05-18T14:15:26Z

@developit what is missing for that to be merged? I could help investing time on it. I understand there are many routers that work with Preact, but since I'm working with preact for performance reasons, I would like to have this base prop to handle my requests on a subdir (it's not about the nested requests, it's about the base prop)

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

d43d920

update to use props.base instead of state.baseUrl

c1931cb

sskoopa added 3 commits December 12, 2016 11:23

move base to this.baseUrl

257a761

adding documentation for nested routes

a8fab3a

fix indentation of nested router doc example

839775c

developit reviewed Dec 15, 2016

View reviewed changes

sskoopa added 9 commits December 15, 2016 14:21

interim work on nested routers

d7e5755

fixup nested route tests in dom

bf1dd22

test nested routers with all path/base combinations

9e4a87b

new Match component, handle var-based paths

3f36e28

attempted test for <Match/>

abb475e

move nested route testing to dom.js

218b127

Router and Match finally work

a6114af

Router and Match finally work

5ac00bd

updated README example

132b302

added ability to reset base. Defining base will not prepend context base

dcdd3f4

developit reviewed Dec 19, 2016

View reviewed changes

developit added the help wanted label Jan 28, 2017

ashsearle mentioned this pull request Feb 6, 2017

Handle relative URLs #138

Closed

merged 2.4.1 changes

07f14d7

Merge from 2.5.1

6b2332f

sskoopa commented Apr 18, 2017

View reviewed changes

final merge of both Match components, nested routers work post 2.5.1

a26343a

developit mentioned this pull request Jul 17, 2017

base URL #212

Closed

laggingreflex mentioned this pull request Oct 5, 2017

Rebase #100 #239

Open

ghost mentioned this pull request Nov 9, 2018

Rebase #100 and #238 #293

Closed

pmkroeker mentioned this pull request Jun 4, 2019

Allow a base path to be set so app can run on a sub directory #279

Open

silentsakky added a commit to silentsakky/preact-router that referenced this pull request Sep 18, 2019

Rebase preactjs#100 with latest changes

a8a3005

silentsakky added a commit to silentsakky/preact-router that referenced this pull request Sep 18, 2019

Rebase preactjs#100 with latest changes

8fff0f4

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

Are you sure you want to change the base?

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

Conversation

sskoopa commented Dec 11, 2016

developit commented Dec 11, 2016

sskoopa commented Dec 12, 2016 • edited Loading

sskoopa commented Dec 12, 2016

developit commented Dec 12, 2016

sskoopa commented Dec 12, 2016

developit commented Dec 12, 2016

sskoopa commented Dec 12, 2016

sskoopa commented Dec 12, 2016

developit commented Dec 12, 2016

developit Dec 15, 2016

Choose a reason for hiding this comment

stonecobra commented Dec 19, 2016 • edited Loading

developit Dec 19, 2016

Choose a reason for hiding this comment

ghost commented Jan 30, 2017

developit commented Jan 30, 2017

osdevisnot commented Feb 17, 2017 • edited Loading

developit commented Feb 17, 2017

sskoopa commented Mar 4, 2017

developit commented Mar 5, 2017

developit commented Apr 17, 2017

sskoopa commented Apr 18, 2017

sskoopa Apr 18, 2017

Choose a reason for hiding this comment

developit Apr 18, 2017

Choose a reason for hiding this comment

sskoopa commented Apr 24, 2017

marcodeltongo commented Jul 29, 2017

chemzqm commented Aug 18, 2017

eligao commented Sep 4, 2017

jahilldev commented May 9, 2018

developit commented Jul 31, 2018

stevengrimaldo commented Oct 21, 2018 • edited Loading

ghost commented Nov 9, 2018

hbroer commented Mar 13, 2019

marvinhagemeister commented Mar 13, 2019

Haugen commented Sep 15, 2020 • edited Loading

developit commented Oct 24, 2020 • edited Loading

renanbandeira commented May 18, 2021

sskoopa commented Dec 12, 2016 •

edited

Loading

stonecobra commented Dec 19, 2016 •

edited

Loading

osdevisnot commented Feb 17, 2017 •

edited

Loading

stevengrimaldo commented Oct 21, 2018 •

edited

Loading

Haugen commented Sep 15, 2020 •

edited

Loading

developit commented Oct 24, 2020 •

edited

Loading