Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

[WIP] Nested routes #308

Merged
merged 46 commits into from
Jul 23, 2018
Merged

[WIP] Nested routes #308

merged 46 commits into from
Jul 23, 2018

Conversation

Rich-Harris
Copy link
Member

This is a big PR with a lot of moving parts. So far all it does is change how routes are created — instead of constructing route objects from an array of filenames (generated from a globbing pattern), it walks over a directory building up the intermediate components it needs. As well as enabling #262, this is arguably much simpler and more robust.

Next task is to change the manifest generation from something like this...

// This file is generated by Sapper — do not edit it!
export const routes = {
  ignore: [/^\/blog.json\/?$/, /^\/blog\/([^\/]+?).json\/?$/],

  pages: [
    { pattern: /^\/?$/, params: () => ({}), load: () => import(/* webpackChunkName: "_" */ '../../routes/index.html') },
    { pattern: /^\/about\/?$/, params: () => ({}), load: () => import(/* webpackChunkName: "about" */ '../../routes/about.html') },
    { pattern: /^\/blog\/?$/, params: () => ({}), load: () => import(/* webpackChunkName: "blog" */ '../../routes/blog/index.html') },
    { pattern: /^\/blog(?:\/([^\/]+?))?\/?$/, params: match => ({ slug: match[1] }), load: () => import(/* webpackChunkName: "blog_$slug$" */ '../../routes/blog/[slug].html') }
  ],

  error: () => import(/* webpackChunkName: '_error' */ '../../routes/_error.html')
};

if (module.hot) {
  import('/Users/208311/Development/SVELTE/sapper/sapper-dev-client.js').then(client => {
    client.connect(10000);
  });
}

...to something like this:

// This file is generated by Sapper — do not edit it!
import root from '../../routes/index.html';

const page_index      = () => import('../../routes/_default.html');
const page_about      = () => import('../../routes/about.html');
const page_blog       = () => import('../../routes/blog.html');
const page_blog_index = () => import('../../routes/blog/_default.html');
const page_blog_$slug = () => import('../../routes/blog/[slug].html');

export const routes = {
  ignore: [/^\/blog.json\/?$/, /^\/blog\/([^\/]+?).json\/?$/],

  root,

  pages: [
    {
      pattern: /^\/?$/,
      parts: [
        { component: page_index }
      ]
    },

    {
      pattern: /^\/about\/?$/,
      parts: [
        { component: page_about }
      ]
    },

    {
      pattern: /^\/blog\/?$/,
      parts: [
        { component: page_blog },
        { component: page_blog_index }
      ]
    },

    {
      pattern: /^\/blog\/([^\/]+?)\/?$/,
      parts: [
        { component: page_blog },
        { component: page_blog_$slug, params: match => ({ slug: match[1] }) }
      ]
    }
  ],

  error: () => import(/* webpackChunkName: '_error' */ '../../routes/_error.html')
};

if (module.hot) {
  import('../../../../sapper/sapper-dev-client').then(client => {
    client.connect(10000);
  });
}

After that, the middleware needs updating to make use of the new server-side manifest, then the runtime needs updating for the client-side manifest.

There are still some details that are subject to bikeshedding, e.g. what to call the property that contains child information for each branch. (I'm leaning towards child — it'd only be a problem if someone explicitly added a child={...} property on the <svelte:component> that introduced a child component, and the docs would tell you not to do that.)

@Rich-Harris
Copy link
Member Author

All tests now passing (locally at least — started getting failures on AppVeyor that appear to be connected to segment-boneyard/nightmare#1476), which makes me very happy. One major piece of missing functionality — if you have a route like

routes/[x]/[y]/[z].html

and you go from /foo/bar/baz to /foo/bar/qux, it re-preloads x and y even though they're unchanged.

Instead, it should keep x and y intact, preload z with the new parameter, set the z component to null then set it back to what it was previously.

Aside from that, there's probably a bit of tidying up to do (e.g. we can improve the manifest slightly, I think) and then it's just a question of documentation.

@Rich-Harris
Copy link
Member Author

Ok, think this is just about ready — it now preserves each unchanged 'level' of component. There's still some tidying up to do but this PR needn't wait on it.

One gotcha that I hadn't anticipated — if you preload data and then update those same keys after the component is rendered, they will get clobbered on route change:

<!-- this will be 43 once oncreate runs, but if the
     user navigates to a new route that preserves
     this component, it will revert to 42 -->
{foo}

<script>
  export default {
    preload() {
      return { foo: 42 };
    },
    oncreate() {
      this.set({ foo: 43 });
    }
  };
</script>

This is potentially a bit annoying but is understandable, and easily worked around. I don't think it's a dealbreaker.

It's so nice being able to do preload at each level of the route hierarchy!

@ansarizafar
Copy link

In my opinion
Index.html should be called Layout,html at every level and must contain

<svelte:component this={child.component} {...child.props}/>

_default.html should be renamed to index.html.

If Layout.html is missing then index.html must be rendered.

@Rich-Harris
Copy link
Member Author

We talked about this in Discord a bit. Nothing is set in stone, but the reason we landed where we did is that the default page is likely to include nested components, and it makes more sense to have a _default directory with an index.html and various NestedComponent.html files than it would to group them inside a directory named index.

There is something appealing about calling index pages index.html though! It'd probably be _layout.html rather than Layout.html, for consistency with other invisible-to-the-router files, especially with _error.html.

On the other hand it feels odd to have preload logic in a 'layout' file.

@TehShrike You convinced me before that index content should go in _default.html or _default/index.html, can you refresh my memory? You've been thinking about this problem for longer than most.

@ansarizafar
Copy link

That is how NuxtJs do nested routing https://nuxtjs.org/guide/routing and here is an example from Nuxt documentation https://nuxtjs.org/examples/nested-routes/

@Rich-Harris
Copy link
Member Author

Yeah, I looked at Nuxt's implementation — I found it very confusing to be honest. The fact that you can have a file called pages/index/index.vue is a bit odd, and it doesn't seem like you can have the equivalent of _layout.html at arbitrary places in the route hierarchy. It's possible I've misunderstood.

The more I think about it, the more I lean towards your suggestion of having index.html always be the leaf nodes in the graph, and using a layout component to customise how things are nested.

Concretely, to adapt this example from the original discussion, that would mean that to achieve an outcome like this for the /settings/notifications/email route...

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

...we'd need the following components:

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

I'm adapting the PR locally and trying that design out on a couple of different apps, to see how natural it feels.

@Rich-Harris
Copy link
Member Author

Another change: it no longer makes sense to have this...

import { routes } from './manifest/client.js';

...since that object contains more than just an array of routes. Instead it is now this (with the equivalent change on the server):

import { manifest } from './manifest/client.js';

@Rich-Harris Rich-Harris merged commit 58de0f9 into master Jul 23, 2018
@Rich-Harris Rich-Harris deleted the gh-262 branch July 23, 2018 01:00
@Rich-Harris
Copy link
Member Author

@ansarizafar Thanks for that last-minute save. I've spent the day thinking about it and I'm very confident that we've made the right decision. Layout components are optional; if omitted, Sapper will use a default layout component:

<svelte:component this={child.component} {...child.props}/>

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants