Feedback from trying out Custom Routes

borekb commented 5 years ago

I noticed that custom routes (RFC #9081) landed in canary a few days ago so yummy, I had to try them!

Overall, it's an awesome feature ❤️. Even in this first pre-release, it allowed me to do a lot so huge congrats to @Timer, @timneutkens, @ijjk and everyone involved. 👍

Params & pre-rendering

As most of our routes are dynamic, I first tried this:

// next.config.js
module.exports = {
  experimental: {
    async rewrites() {
      return [
        { source: '/post/:id', destination: '/post' },
      ];
    },
  },
};

// pages/post.js
import { useRouter } from 'next/router';

const Post = () => {
  const { query } = useRouter();
  return <h1>Post {query.id}</h1>;
};

export default Post;

This should be roughly equivalent to a dynamically-routed component pages/post/[id].js which would have this behavior:

There's no getInitialProps so this page is pre-rendered to HTML.
During SSR, the query object is {}.
On the client-side, the app hydrates and JavaScript "fixes" the UI.

With custom routes, the third step doesn't happen which left me initially confused – I thought my rewrite rules are wrong. If this could be aligned with dynamic routing's behavior, with the same set of tradeoffs, that would be great. 👍

The fix / workaround is to add getInitialProps. (Here's a related test; "GIPGIP" actually has a lot of significance 😄.)

Dynamic routing

I initially thought that dynamic routing & rewrites don't work together but that was only because of the pre-rendering issue above. This works fine:

// next.config.js
module.exports = {
  experimental: {
    async rewrites() {
      return [
        { source: '/c/:slug', destination: '/category/:slug' }
      ];
    },
  },
};

// pages/category/[slug].js
import { useRouter } from 'next/router';

const Category = () => {
  const { query } = useRouter();
  return <h1>Category {query.slug}</h1>;
};

// ❗ Don't forget to opt out of pre-rendering
Category.getInitialProps = () => ({});

export default Category;

Partial path matching doesn't work with non-dynamic page components

This might be a bug, I'm not sure.

Given the two components from above, this is what I'm seeing:

// pages/category/[slug].js; ✅ works fine:
{ source: '/category-:slug', destination: '/category/:slug' },

// pages/post.js; 🚫 doesn't work:
{ source: '/post-:id', destination: '/post' },

// full path segment works:
{ source: '/post/:id', destination: '/post' }

Console errors

When rewriting goes wrong and the result is 404, it would be nice to see some sort of info in the console.

Future routing wishes

This section is not strictly about Custom Routes but while I'm writing this, I wanted to mention that we're dealing with complex routing requirements in an e-commerce solution we're working on. The users can choose to use vanity URLs like /nike or /adidas-air which makes it impossible to tell just from the path which page component should be used.

Some assorted thoughts:

Custom Routes would (I think) allow us to fetch all possible routes at build-time, create a possibly large array of rewrite rules and do routing entirely in Next.js. The downside is that the users of the platform can add new products / categories etc. at any point so those would not resolve.
If the route resolution could be dynamic somehow, without requiring custom server in front of it, that would be fantastic. This might be against "serverless" though, I'm not sure.
If proxy (load balancer) was doing path rewriting, I'd worry that if a browser makes a request like /three/levels/deep but Next.js only sees it as /two/levels, things like hrefs could be off. But again, I'd have to try this.

Overall, thanks for all the improvements in the routing department! You're doing great work. 👏

Timer commented 5 years ago

Wow, thank you for this feedback. We'll definitely address each one of these feedback points in the coming days.

borekb commented 5 years ago

Thanks, here is some more:

"Stop here" flag

UPDATE: I now think it would be better to have non-cascading behavior by default, like routing in now.json. See the discussion below with @Janpot.

Sometimes, I want to finish immediately so that I don't need to worry about interactions with other rewrite rules below. Something like this would be useful:

{ source: '/post/:id', destination: '/post', stopHere: true }

Apache calls this the "last" flag – [L] which is probably a better name 😄.

Catch-all rule

I'd like to implement a catch-all rule like this:

{ source: '/(.*)', destination: '/catchall' }

I'd use that component to check if the route exists (fetch from backend) and display the appropriate component (or 404 page).

I think it's currently impossible as all rules will eventually reduce down to this catch-all rule. The "stop here" feature would probably help resolve this.

Note: this comment https://github.com/zeit/next.js/issues/8053#issuecomment-542654273 indicates that the Custom Routes RFC should indeed handle catch-all behavior.

Routing based on HTTP headers?

The catch-all route above would be useful for vanity / marketing URLs like this: https://example.com/any-string-really. Another solution in our case would be to route based on HTTP headers because the load balancer adds a small HTTP header to each request:

GET /any-string-really
X-Page-Type: product

So when the request reaches Next.js, there's enough information in it about which page component to render and maybe, the next.config.js could support something like this:

{
  source: {
    httpHeaders: 'X-Page-Type: :pageComponent',
    path: '/:id',
  },
  destination: '/:pageComponent/:id',
}

Or even less dynamic would be fine too:

{
  source: {
    httpHeaders: 'X-Page-Type: product',
    path: '/:id',
  },
  destination: '/product/:id',
},
{
  source: {
    httpHeaders: 'X-Page-Type: category',
    path: '/:id',
  },
  destination: '/category/:id',
},
// ...

I'm trying to avoid custom server as much as possible 😄.

Janpot commented 5 years ago

@borekb Wouldn't the custom _error page be enough as a catch-all mechanism?

Personally, I feel like all these rule modifiers in apache have turned it into some sort of half-baked pseudo programming language. In my experience, the effect of this is that people try to cram as much logic in them as they can and turn it into a hard to debug black boxes of routing logic.

borekb commented 5 years ago

@Janpot I agree in general, there needs to be a balance.

With the "stop here" mechanism, I'd argue that it actually makes reasoning about the rules easier.

As for catch-all, it's a routing feature to me, even if it could be emulated via _error. Generally, I believe that the more the system knows about routing, the more it can optimize the deployments. Then there is also the question of DX / intuitiveness and I would be looking for a catch-all feature in the routing layer.

As for HTTP headers, they're in the "not likely" department even in my head 😄. They would be a powerful feature but also possibly complex and with benefits for only a small number of users. As long as there is a good solution for vanity URLs (and custom server is not that), I'm happy.

Janpot commented 5 years ago

With the "stop here" mechanism, I'd argue that it actually makes reasoning about the rules easier.

But if the catch-all case is solved, would there still be a need for "stop here"? I can't think of another case that needs "stop here" that wouldn't be solvable with the current RFC.

borekb commented 5 years ago

It's purely about reasoning about the rules.

In fact, when we're talking about it, I just realized that Now routing doesn't cascade by default, so when I'm reading the now.json and find a match, I know how the request is routed straight away. When I was reading the Next.js routing in the tests, it was mentally harder to tell how a request will be routed, and I even guessed incorrectly for the /test-overwrite/a/b route.

With Now routing, you have to explicitly pass continue: true which I actually quite like.

Janpot commented 5 years ago

🤔 I see. I was not very familiar with now routing.

borekb commented 5 years ago

Next round:

Custom + dynamic routes don't work on Now

I have a demo app with two page components, one "normal", one dynamic:

pages/product.js
pages/category/[id].js

Locally, both work fine but when deployed to Now with @now/next@1.0.6-canary.0, only the product routes work – categories don't. You can try it here: https://nextjs-custom-routes-a0pz7flvg.now.sh/

Catch-all via `_error.js`

Motivated by the discussion above, I tried implementing dynamic route resolution via _error.js, it looks like this:

import React from 'react';
import { useRouter } from 'next/router';
import NextError from 'next/error';
import dynamic from 'next/dynamic';
const ProductPage = dynamic(() => import('./product'));
const CategoryPage = dynamic(() => import('./category/[slug]'));

function Error({ pageToRender, route }) {
  const router = useRouter();

  switch (pageToRender) {
    case 'category':
      router.query.slug = route.substring(1); // quick & dirty
      return <CategoryPage />;
    case 'product':
      router.query.id = route.substring(1);
      return <ProductPage />;
    default:
      return <NextError statusCode={404} />;
  }
}

Error.getInitialProps = async ({ req, res }) => {
  const pageToRender = await resolvePageType(req.url);
  res.statusCode = pageToRender ? 200 : 404;
  return { pageToRender, route: req.url };
};

async function resolvePageType(route) {
  // would fetch from backend...
  return {
    '/dynamically-resolved-category': 'category',
    '/dynamically-resolved-product': 'product',
  }[route];
}

export default Error;

It mostly works, just in dev mode, the browser keeps reloading routes that fall back to _error.js. Production build seems to be fine.

Still, catch-all feels like it should be a routing feature, not this hack 😄.

Janpot commented 4 years ago

yeah, I agree, that's less than ideal.

Fumler commented 4 years ago

Thanks, here is some more:

...

Routing based on HTTP headers?

The catch-all route above would be useful for vanity / marketing URLs like this: https://example.com/any-string-really. Another solution in our case would be to route based on HTTP headers because the load balancer adds a small HTTP header to each request:
GET /any-string-really
X-Page-Type: product
So when the request reaches Next.js, there's enough information in it about which page component to render and maybe, the next.config.js could support something like this:
{
  source: {
    httpHeaders: 'X-Page-Type: :pageComponent',
    path: '/:id',
  },
  destination: '/:pageComponent/:id',
}
Or even less dynamic would be fine too:
{
  source: {
    httpHeaders: 'X-Page-Type: product',
    path: '/:id',
  },
  destination: '/product/:id',
},
{
  source: {
    httpHeaders: 'X-Page-Type: category',
    path: '/:id',
  },
  destination: '/category/:id',
},
// ...
I'm trying to avoid custom server as much as possible 😄.

Just want to add to the discussion that we also use a custom server mostly for something like that.

Our pages structure:

pages/
    www.site1.com/
        index.tsx
    www.site2.com/
        index.tsx

and in our custom server:

server.get('/', (req, res) => {
    return app.render(req, res, `/${req.header('x-site')}`) // or could be req.header('host')
})

This let's us have site specific pages but still have "normal" routes, so we would very much like this feature 👼

dohomi commented 4 years ago

Hey Guys,

I was experimenting with the new feature on canary-8. I set up one rewrite rule to fetch all urls with one file (I use a headless CMS):

serverless: true,
experimental: {
      async rewrites () {
        return [
          {source: '/:slug+', destination: '/index'}
        ]
      }
    },

Everything works as expected, but every click on a link is now a full page reload and not a client-side route. Is this a bug or am I overlooking something?

dohomi commented 4 years ago

I just updated to 9.1.4 and I combined the new catchAllRouting and rewrites as following:

experimental:{
async rewrites () {
        return [
          {source: '/', destination: '/[...index]'}
        ]
      },
      catchAllRouting: true
}

Below the issue I am facing, basically all links work server and client side but only the link to the root of the page is not working. I can remove the as or href but then the link would require a page reload

<Link as={as} href="/[...index]" /> // => works perfectly for /path, /path/subpath
<Link as="/" href="/[...index]" /> // => throws error on client side routing. SSR works

With this setup only one [...index].tsx file is needed to serve all requests. Everything works great except the little issue with the root link.

borekb commented 4 years ago

Wow, catch-all / "...rest" exists already? Missed #9416, that's great news!

borekb commented 4 years ago

@dohomi How does the [...index].tsx impact chunking?

dohomi commented 4 years ago

@borekb not sure.. I just started playing around with it. I honestly think that for my case (working with a headless API approach) it should not matter at all. I just use a second lambda which simply re-export the [...index].tsx content like following:

// index.tsx
import INDEX from './[...index].tsx'
export default INDEX

Now I could remove the rewrites section and my client as well as server side routing was working as expected. Would love to hear some feedback from the core devs if thats the approach to take

Timer commented 4 years ago

@dohomi How does the [...index].tsx impact chunking?

Very badly. The catch-all route should never be used to mount a subrouter in your application.

It's strictly meant for CMS use cases, the RFC is being posted soon!

dohomi commented 4 years ago

Very badly. The catch-all route should never be used to mount a subrouter in your application.

It's strictly meant for CMS use cases, the RFC is being posted soon!

@Timer what do you mean by very badly? I ended up using https://github.com/zeit/next.js/issues/9390#issuecomment-555310907 so I only have one single [...index].tsx file which serves all requests of an endpoint connected to a CMS API. It feels pretty solid though..

timneutkens commented 4 years ago

@dohomi it means, there is no code splitting besides our chunking mechanism, meaning you're more likely to "overship" code as you're routing 100% of routes to one page, so that bundle will be very large depending on how you set it up.

For example if you route all routes to a single page you're likely to include multiple different layouts etc.

Unless it's something very basic and there is only one page view and all content is the exact same type from a CMS. With the exact same type I mean you don't have multiple different "post types", like in WordPress you could have a blog + pages and those both warrant different layouts. For that case it's better to create multiple pages like for example:

/blog/[...slug].js and /[...slug].js this would mean /blog/hello-world gets served a different bundle than /support for example.

dohomi commented 4 years ago

@timneutkens thanks for your explanation. As the layout and content blocks are all based on the JSON output of the headless API I think I have to bite the bullet and "overship" code because of the unknown amount and type of content needs to be delivered to the frontend. I use React.createElement(VARIOUS_CONTENT_TYPE, PROPS) to render every content block and layout section dynamically. I can post a website based on this approach if you guys are interested in the outcome.

poupryc commented 4 years ago

Hello, I think some points raised in this feedback should be answered with the latest version of path-to-regexp. Being a major version of path-to-regexp, I guess it would be necessary to rewrite the implementation of custom routes, but it's worth it. Looking forward to see that!

brunomarchioro commented 4 years ago

Hello, I started testing this feature like the @dohomi code above. With /[...index] to take all routes. However, internal routes, such as favicon.ico and other resources, are captured. Is this the expected behavior?

balazsorban44 commented 2 years ago

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

vercel / next.js