sveltejs / sapper

The next small thing in web development, powered by Svelte
https://sapper.svelte.dev
MIT License
6.99k stars 432 forks source link

Add support for nested routes #262

Closed tony-sull closed 6 years ago

tony-sull commented 6 years ago

This one came up in Gitter today and seems worth investigating. It could be really helpful for Sapper to support nested routing, potentially with a special slot, <sapper:child /> or similar.

Sapper currently handles routes by walking the routes directory and finding the matching path (/settings/profile.html or /settings/profile/index.html for example). In cases where you want a shared Settings layout that changes content, you currently need to create something like a SettingsLayout.html component and use that to wrap content in /settings/profile.html. i.e. The child route needs to know all parent components and include them in it's layout.

This gets a little tricky with multiple levels of nested routes. Ember, Angular, and React Router have slightly different solutions for nesting routes - worth investigating how they solved it and what makes the most sense for Svelte/Sapper

tony-sull commented 6 years ago

Links to how some of the other frameworks handle nested routing:

Vue https://router.vuejs.org/en/essentials/nested-routes.html Ember https://guides.emberjs.com/v2.11.0/tutorial/subroutes/ React Router https://github.com/reactjs/react-router-tutorial/tree/master/lessons/04-nested-routes Angular https://angular.io/guide/router

Rich-Harris commented 6 years ago

Been reading the linked docs and, well, hoo boy. I can't believe it's nearly as complicated as Vue, Ember and Angular make it out to be, with special elements and extra configuration all over the place.

React Router has by far the most sensible solution to this problem, though the design of RR is fundamentally at odds with that of Sapper.

I think the solution is two-fold:

  1. Add some way of naming a page that says 'match multiple path parts'
  2. Use #if blocks in the template

By which I mean that we could have a page like routes/settings/[+]/index.html, it would match /settings, /settings/notifications, /settings/notifications/emails and so on.

That file could look like this:

<h1>Settings</h1>

{#if menu[0] === 'notifications'}
  <Notifications {menu}/>
{:else}
  <!-- ... -->
{/if}

<script>
  export default {
    components: {
      Notifications: './_components/Notifications.html'
    },

    preload({ path }) {
      return {
        menu: path.split('/')
      };
    }
  };
</script>

(Note that if we change the default behaviour outlined in #295, whereby the page component persists between navigations if the constructor is unchanged, then we would need to implement something like the persist option discussed therein.)

Am I being naive? Or would that cover the cases we care about?

silentworks commented 6 years ago

Your solution sounds like a lot of manual work, where-as Ember and Vue is not so manual.

Rich-Harris commented 6 years ago

Huh. Genuinely surprised at that reaction — in Vue we have to add this kind of boilerplate...

const router = new VueRouter({
  routes: [
    { path: '/user/:id', component: User,
      children: [
        {
          // UserProfile will be rendered inside User's <router-view>
          // when /user/:id/profile is matched
          path: 'profile',
          component: UserProfile
        },
        {
          // UserPosts will be rendered inside User's <router-view>
          // when /user/:id/posts is matched
          path: 'posts',
          component: UserPosts
        }
      ]
    }
  ]
})

...and Ember has all this gubbins, which you have to maintain separately from the route templates/controllers themselves, and which involves knowledge of the router API:

Router.map(function() {
  this.route('about');
  this.route('contact');
  this.route('rentals', function() {
    this.route('show');
    this.route('show', { path: '/:rental_id' });
  });
});

Do you mean that the if...else stuff is potentially a lot of manual labour? My thinking is that if it's purely based on props.path then you can tackle that stuff however you want — with if...else, or with <svelte:component this={SubMenu}> (where SubMenu is a computed property), or whatever else you can dream up, all without learning new stuff other than [+].

@tehshrike is typing some interesting ideas into Discord as we speak though, which may simplify things further...

arxpoetica commented 6 years ago

I like @Rich-Harris's proposal. Question: what happens if one wants to override a particular nesting. In your scenario:

Give that routes/settings/[+]/index.html matches:

What if one creates an actual new nested component index.html file at:

routes/settings/some-new-nesting/index.html

What takes precedence?

Rich-Harris commented 6 years ago

@arxpoetica routes/settings/some-new-nesting/index.html would take precedence, same as it currently would if you had that alongside routes/settings/[submenu]/index.html

arxpoetica commented 6 years ago

Just so we capture, this from @TehShrike

I don't recall exactly how nested directories are handled now, so it might be a big breaking change, but how I initially thought Sapper worked (and how I still think would be a good idea) would be for the file /menu/notifications/index.html component to be nested inside the /menu/index.html component when the user visited site.com/menu/notifications. That would only require a slot-like element to tell Sapper where in the menu component the current child should be nested. I suppose it could literally be a named slot.

it would presumably be a named slot with a name that made sense, like <slot name="route"></slot> or something

tony-sull commented 6 years ago

I had a similar idea as @TehShrike on that one, I'm partial to it being a convention-based solution. There are plenty of scenarios where an app may need to nest routes and have the child route hosted in the parent's container. I think the original solution there was a Layout component, but that got a bit unwieldy if I remember right.

As for how it handles precedence, I'd say something like this for the route /routes/settings/account/index.html

Does the parent route, /routes/settings/index.html in this case, have a slot for sub-routes? If yes, render the parent while doing the same check for it's parent route, then slot in the sub-route If no, just render /routes/settings/account/index.html and ignore any parent routes

The only interesting case I can think of would be something like /routes/settings/account.html. Should it check /routes/settings/index.html for a sub-route slot?

Rich-Harris commented 6 years ago

Now that @tehshrike has opened my eyes, I'm going to try and practice what I preach and think about this from a user perspective rather than an implementer perspective: it would be nice if it worked that way. Among the benefits:

Presumably it would look something like this:

<!-- routes/settings/html -->
<h1>Settings</h1>

<Sidebar/>

<div class="submenu">
  <sapper:view foo={bar}/>
</div>

That would be roughly equivalent to this...

<!-- routes/settings/html -->
<h1>Settings</h1>

<Sidebar/>

<div class="submenu">
  <svelte:component this={Submenu} foo={bar}/>
</div>

...except that Submenu would be lazily loaded. (Because everything is statically analyzable, we wouldn't need to wait to load routes/settings.html before loading routes/settings/notifications.html; we could load the two things in parallel.) In all likelihood that's how we'd implement it as well; the main challenge would be figuring out how to inject Submenu into a nested component.

So, what do we reckon? Is this the right approach?

Rich-Harris commented 6 years ago

@tonyfsullivan missed your post while I was replying! To this point:

The only interesting case I can think of would be something like /routes/settings/account.html. Should it check /routes/settings/index.html for a sub-route slot?

Yes, I think so — elsewhere in Sapper /routes/settings/account.html and /routes/settings/account/index.html are functionally identical.

Rich-Harris commented 6 years ago

A further thought, not entirely finished — would we need app/App.html if we had this? Or would routes/index.html serve the same need?

johanalkstal commented 6 years ago

I like the potential simplicity of the idea of /routes/page/index.html being the "Layout" component of subroutes if it has a kind of <childslot></childslot> component in it.

It's elegant.

johanalkstal commented 6 years ago

@Rich-Harris Is there a way right now to "opt-out" of the App.html component?

Some pages might not require the overall layout of all other pages, for instance log in pages or special landing pages or whatnot.

In that regard, not having App.html would be beneficial and you could decide which "layout" to use for which page with a parent index.html file.

tony-sull commented 6 years ago

I do think we could get rid of app/App.html with this approach. To @johanalkstal's question, if we scrap App.html then this approach would allow apps to share /routes/index.html when desired but also do things like /routes/dashboard/index.html' and/routes/settings/index.html' where they act as unique wrapper layouts for each module of the app

paulocoghi commented 6 years ago

What I will say here probably was discussed on other issues here and/or on Svelte repository.

I really like the following approach, that I already develop on top of Svelte, in which we pass the routes and their components.

IMHO, this approach can solve a lot of common issues with routes like: nested routes, reused components on different routes, different routes with the same components, etc.

Do you think this approach might be interesting for Sapper? If so, I will be happy to study Sapper thoroughly to bring my implementation to it (for example, as an option).

Note: I understand that my suggestion is arriving too late, as Sapper has already followed another model to define and organize the app routes and its components. But share your opinions.

Basic case:

app.areas = [ 'content' ];

// This will automatically load the component "components/HelloWorld.html" in the route "/"
app.route[ '/' ] = {
    content: 'HelloWorld'
};

Multiple area and components with reused components

app.areas = [ 'content', 'footer' ];

// On this example the component "components/HelloWorld.html"
// will not be "re-rendered" when switching from the route "/" to "/other"
// and vice-versa.

app.route[ '/' ] = {
    content: 'HelloWorld',
    footer: 'Footer1'
};

app.route[ '/other' ] = {
    content: 'HelloWorld',
    footer: 'Footer2'
};

Nested routes, that I named "route groups"

app.areas = [ 'header', 'content', 'footer' ];

app.group[ '/settings' ] = {

    // This is the root route for "/settings"
    '/': {
        header: 'Menu'
        content: 'Settings/Main' //components/Settings/Main.html
        footer: 'Footer'

    }

    // This is the route "/settings/notifications"
    '/notifications': {
        header: 'Menu'
        content: 'Settings/Notifications' //components/Settings/Notifications.html
        footer: 'Footer'

    }
};

I have different use cases solved with this approach, which if I put here would make the list extensive, but I am available to expose other cases whenever requested.

That's it :)

paulocoghi commented 6 years ago

With the approach I explained above, the location of the component files doesn't matter.

In my implementation I defined a root folder components, and you can organize them the way you like, independently from where they are used.

paulocoghi commented 6 years ago

The technical part of my implementation is that, since we have all the app's routes and components, we can programmatically create the code for the "main" app (also with Svelte) that will lazily load the respective components for each route, including all sub-components without having to specify them on the main "map".

silentworks commented 6 years ago

@paulocoghi what you are suggesting is what Absolute State Router does, the only issue with ASR at the moment is that it doesn't support Server Side Rendering. But as soon as that is done, there is no reason why someone wouldn't be able to create their own Sapper like setup with it.

paulocoghi commented 6 years ago

@silentworks, more than suggesting, its already done. My suggestion is to merge my work with Sapper (if @Rich-Harris understand that this model can be complementary to the existing one on Sapper).

No problem if the conclusion is that these two models are considered very different and there isn't much sense in merging them.

;)

Rich-Harris commented 6 years ago

@johanalkstal you can't currently opt out of App.html, but you can customise its behaviour however you like:

<!-- App.html -->
{#if props.path.startsWith('/special')}
  <SpecialLayout>
    <svelte:component this={Page} {...props}/>
  </SpecialLayout>
{:else}
  <NormalLayout>
    <svelte:component this={Page} {...props}/>
  </NormalLayout>
{/if}

It seems like we probably could get rid of it if we implement <svelte:view> though. (I'm not sure I like calling it that — better suggestions welcome. <svelte:inject>?)

@paulocoghi I think there's merit in the approach you describe, but I'm afraid I also think it's fundamentally at odds with Sapper's design:

In my implementation I defined a root folder components, and you can organize them the way you like, independently from where they are used.

To me, the whole value of the Next/Nuxt/Sapper approach is that you can't put components wherever you want — there's a clear correct place to put them that means anyone familiar with the framework can instantly understand the codebase. Using the filesystem as a single source of truth is also beneficial from the point of view of code-splitting and generating efficient code (part of the reason Sapper's runtime is so small is that the 'router' is essentially a bunch of regexes in the app/manifest folder).


I think I'm slowly figuring out how all this would work in practice — will see if I can open an RFC on Svelte itself, since a lot of the work would need to happen there.

arxpoetica commented 6 years ago

<svelte:layout>? <svelte:template>? <svelte:nest>?

Rich-Harris commented 6 years ago

Ok, so to summarise the chats we've been having in Discord, We Have A Plan.

Rather than introducing a new primitive, the proposal is to make some significant (but straightforward) changes for Sapper 0.15 that will result in no changes to Svelte. Basically, we go from having a flat route structure, where /settings, /settings/notifications and /settings/notifications/email are all possible values of Page, to be applied to the top-level App.html component, to having a hierarchical structure:

// pseudo-code representing what happens in sapper/runtime.js
const [
  { default: App },
  { default: Settings },
  { default: Notifications },
  { default: Email }
] = Promise.all([
  await import('routes/index.html'),
  await import('routes/settings/index.html'),
  await import('routes/settings/notifications/index.html'),
  await import('routes/settings/notifications/email.html')
]);

const [
  data,
  settingsData,
  notificationsData,
  emailData
] = Promise.all([
  App.preload ? await App.preload(...) : {},
  Settings.preload ? await Settings.preload(...) : {},
  Notifications.preload ? await Notifications.preload(...) : {},
  Email.preload ? await Email.preload(...) : {}
]);

const app = new App({
  ...data,
  sapper: {
    child: Settings,
    props: {
      ...settingsData,
      sapper: {
        child: Notifications,
        props: {
          ...notificationsData,
          sapper: {
            child: Email,
            props: emailData
          }
        }
      }
    }
  }
});

Then, your pages could look like this:

<!-- routes/index.html -->
<svelte:component this={sapper.child} {...sapper.props}/>
<!-- routes/settings/index.html -->
<h1>Settings</h1>
<svelte:component this={sapper.child} {...sapper.props}/>
<!-- routes/settings/notifications/index.html -->
<h2>Notifications</h2>
<svelte:component this={sapper.child} {...sapper.props}/>
<!-- routes/settings/notifications/email.html -->
<h3>Email</h3>
<label>
  <input type=checkbox bind:checked=$emailNotifications>
  email notifications
</label>

This would render like so:

<h1>Settings</h1>
<h2>Notifications</h2>
<h3>Email</h3>
<label>
  <input type=checkbox>
  email notifications
</label>

If we then navigated to /settings/notifications/desktop...

<!-- routes/settings/notifications/desktop.html -->
<h3>Desktop</h3>
<label>
  <input type=checkbox bind:checked=$desktopNotifications>
  desktop notifications
</label>

...then this would happen:

const { default: Desktop } = await import('routes/settings/notifications/desktop.html');
const desktopData = await Desktop.preload(...);

const state = app.get();
state.sapper.props.sapper.props.sapper.child = Desktop;
state.sapper.props.sapper.props.sapper.props = desktopData;
app.set(state);

The <h1> and <h2> would be left alone; the <h3> and everything below it would be replaced.

I like this approach because it's nice and explicit. It's consistent, it allows us to do granular code-splitting etc, it lets us use preload at every level of the hierarchy, and it means we can do away with App.html.

Preloading logic

In order to prevent component state from getting corrupted, I think it's important that each level of the hierarchy only has access to parameters that are 'in scope' in preload. So in the case of /products/[category]/[id] (notice that Category.preload is called with the category param, while Id.preload is called with category and id params):

const [
  { default: App },
  { default: Products },
  { default: Category },
  { default: Id }
] = Promise.all([
  await import('routes/index.html'),
  await import('routes/settings/index.html'),
  await import('routes/settings/notifications/index.html'),
  await import('routes/settings/notifications/email.html')
]);

const [
  data,
  productsData,
  categoryData,
  idData
] = Promise.all([
  App.preload ? await App.preload({ params: {}, ... }) : {},
  Products.preload ? await Products.preload({ params: {}, ... }) : {},
  Category.preload ? await Category.preload({ params: { category: 'beer' }, ... }) : {},
  Id.preload ? await Id.preload({ params: { category: 'beer', id: '123' }, ... }) : {}
]);

Navigating to the 'same' page

If it were a case of navigating from /blog/post-one to /blog/post-two — i.e. it's the same routes/blog/[slug].html page in both cases — we would need to set state.sapper.props.sapper.child to null before setting it to Slug so that lifecycle stuff was correctly observed.


Thanks everyone for guiding this process, and for bearing with me while I stumbled towards a design that feels like it could actually work. For the first time I don't have any reservations! If you feel otherwise, speak up, because I'm planning to implement this soon.

Rich-Harris commented 6 years ago

A further thought: a very common requirement is to indicate which sub-route is selected in the parent route:

<!-- routes/foo/index.html -->
<Sidebar selected={???}/>
<svelte:component this={sapper.child} {...sapper.props}/>
<!-- routes/foo/_components/Sidebar.html -->
<a href="foo/bar" class="{selected === 'bar' ? 'selected' : ''}">bar</a>
<a href="foo/baz" class="{selected === 'baz' ? 'selected' : ''}">baz</a>
<a href="foo/bop" class="{selected === 'bop' ? 'selected' : ''}">bop</a>

It seems like it'd be useful to make that information available in a way that doesn't require the developer to do something complicated involving props.path and computed properties.

Maybe something like this?

<Sidebar selected={sapper.childpathpart}/>

What would be a better name than childpathpart, where it equates to bar, baz or bop in the context of routes/foo/index.html? Actually let me rephrase that, since the answer is 'just about anything': what would be a good name?

Rich-Harris commented 6 years ago

Apparently the correct term is 'segment', which makes sense: https://english.stackexchange.com/questions/381553/what-could-a-part-of-path-of-url-be-called

So maybe sapper.segment? Or does that imply foo in the context of routes/foo/index.html, rather than bar, baz or bop?

In fact perhaps it'd be more logical to have this...

<!-- routes/foo/index.html -->
<Sidebar selected={child.segment}/>
<svelte:component this={child.component} {...child.props}/>

...though child isn't really viable as a reserved property.

ekhaled commented 6 years ago

segments needs to be an array really.

Imagine hierarchical menu structures.

Each menu in the hierarchy can then just be concerned with it's own level segments[0], segments[1] etc.

Or maybe some other sort of hierarchical structure.

akaufmann commented 6 years ago

What I don't like so much about <sapper|child>.segment is, that you don't know what the context is if you are new to Sapper. It doesn't tell you that this is a segment of a URI.

How about... sapper.activeRoutes: {[key: string]: boolean} = {'products': true, 'ratings': true} for e.g. routes/products/[id]/ratings.html ?

Rich-Harris commented 6 years ago

@ekhaled I think that in most cases you just want the next segment, but if you do need all segments then it can easily be constructed from path, which is available throughout the app:

<ul>
  {#each segments as segment}
    <ol>{segment}</ol>
  {/each}
</ul>

<script>
  export default {
    computed: {
      segments: ({ path }) => path.split('/').filter(Boolean)
    }
  };
</script>

@akaufmann I think that probably holds true whatever we do — it's just something we have to do a good job of documenting. activeRoutes is an interesting idea, though it arguably adds some extra ambiguity (is [id] included, for example?)

The PR is ready (https://github.com/sveltejs/sapper/pull/308), I'm going to merge it and try to get a release out soon, so if anyone has any major objections to the approach we've taken here then now is the time to share them!

Rich-Harris commented 6 years ago

Alright everyone, I've released Sapper 0.15:

Thanks everyone for shaping this feature. It took a long time for us to get there but I'm extremely pleased with how it turned out. It's now very easy to do things with Sapper that are prohibitively difficult with similar frameworks.

davidwparker commented 4 years ago

In case anyone is looking for the docs- it's here now: https://sapper.svelte.dev/docs#Layouts

And segment may be going away: https://github.com/sveltejs/sapper/issues/824