Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Catch-all routes do not catch the root index route #10488

Closed
flybayer opened this issue Feb 11, 2020 · 38 comments · Fixed by #12887
Closed

Catch-all routes do not catch the root index route #10488

flybayer opened this issue Feb 11, 2020 · 38 comments · Fixed by #12887

Comments

@flybayer
Copy link
Contributor

flybayer commented Feb 11, 2020

Feature Request

Describe the use case

I have a use case where I need all of the following requests to go to the same function:

POST pages/api/users
PATCH pages/api/users/1
DELETE pages/api/users/1

After reading the docs, I was sure I could catch all of those using this:

pages/api/users/[...slug].js

However, this does not catch POST pages/api/users without an explicit /index.js.

To Reproduce

Try the above scenario (even with GET requests)

Expected behavior

This route:

pages/api/users/[...slug].js

Catches all of these:

POST pages/api/users
PATCH pages/api/users/1
DELETE pages/api/users/1

Screenshots

N/A

System information

N/A

Additional context

Next.js 9.2.2-canary.15

@Janpot
Copy link
Contributor

Janpot commented Feb 11, 2020

And it seems like, even if I try to add a

// /api/users/index.js
export { default } from "./[...slug]";

It makes GET /pages/api/users work, but GET /pages/api/users/ gives me a 500. Without the index file that route 404s.

@timneutkens
Copy link
Member

With regards to the initial issue, this is expected behavior, catch-all means the segment you define is a catch-all. What you're referring is an optional segment where slug is not required in all cases which is currently not supported. Instead you can do what @Janpot suggested. Not sure why it gives a 500 on that specific route though, @Janpot could you PR a test case?

@Janpot
Copy link
Contributor

Janpot commented Feb 11, 2020

@timneutkens I just tried it in a codesandbox but I'll see if I can PR something tonight or tomorrow.

@flybayer
Copy link
Contributor Author

flybayer commented Feb 11, 2020

@timneutkens ok, is there a possibility to add this feature somehow? I'm working on a modern Rails equivalent for Next.js, and like a Rails controller, I want a single file for handling all my CRUD operations. I'd really like to avoid cluttering the file system with a bunch of re-export files. This type of REST API is so common, it seems like a great feature candidate.

Something else I intuitively tried was:

pages/api/users[...slug].js

@timneutkens
Copy link
Member

I'll keep the issue open to see if there's anyone else asking for it.

@Janpot
Copy link
Contributor

Janpot commented Feb 12, 2020

@timneutkens created #10502

Been thinking a bit more about this issue and I think the requested behavior could make more sense for the following reasons:

  1. Conceptually [...slug] looks like array destructuring into an array. Arrays can have zero items, so why can't slug have zero items? The index route would just receive an empty array as the slug parameter.

  2. The solution I proposed is flawed. Let me give an example:

Current situation, imagine we have a catch-all route like:

// /api/posts/[...slug].js
export default function (req, res) {
  res.send(getPost(req.query.slug))
}

And we fetch the following urls:

url getPost called with
/api/posts 404 wtf? /api/posts/index resolves, so why not this?
/api/posts/index ['index'] ok
/api/posts/something ['something'] ok
/api/posts/something/else ['something', 'else'] ok

Imagine we add a index route:

// /api/posts/index.js
export { default } from './[...slug]';

now this behavior becomes

url getPost called with
/api/posts undefined ok (I guess)
/api/posts/index undefined wtf?
/api/posts/something ['something'] ok
/api/posts/something/else ['something', 'else'] ok

Now, imagine /api/posts/[...slug].js would also be called for the index route, like this issue proposes. Then with just the following route

// /api/posts/[...slug].js
export default function (req, res) {
  res.send(getPost(req.query.slug))
}

the behavior would be more predictable and uniform and zen

url getPost called with
/api/posts [] ok
/api/posts/index ['index'] ok
/api/posts/something ['something'] ok
/api/posts/something/else ['something', 'else'] ok

@flybayer
Copy link
Contributor Author

@Janpot, Yes!! you very eloquently described how I'm thinking about this.

@eddyw
Copy link

eddyw commented Feb 14, 2020

+1 having this same issue as well as the same 500 issue when doing export { default } from "./[...slug]" on index.tsx

Having this setup:

pages/
- api/
-- [...fragment].tsx
- [...fragment].tsx

It doesn't match / root path. However, as a workaround, this works locally with next dev if you add to next.config.js

  exportPathMap: async function (defaultPathMap) {
    return merge({}, defaultPathMap, {
      '/': { page: '/[...fragment]', query: { fragment: ['/'] } },
    })
  },

So this correctly returns as @Janpot described on last table. However, I'm deploying to Zeit Now and I couldn't figure how to make that work since exportPathMap only works with next dev (and/or next export). This is serverless, so exportPathMap is ignored.

Any update on this? or does anyone have a better workaround for the time being to make this work with now deployments? Tried rewrittes but I couldn't make any sense of how it's supposed to work 🙈

@flybayer
Copy link
Contributor Author

Now that blitz is in such high demand, could this feature be approved?

Below are the files for my tiny blog demo. The lack of this feature literally doubles the files needed. All of the index files are just re-exports of [...id]

Currently:

pages/api/posts/[...id].js
pages/api/posts/index.js
pages/api/posts/[postId]/comments/[...id].js
pages/api/posts/[postId]/comments/index.js
pages/api/comments/[...id].js
pages/api/comments/index.js

After this feature:

pages/api/posts/[...id].js
pages/api/posts/[postId]/comments/[...id].js
pages/api/comments/[...id].js

@yanv1991
Copy link

yanv1991 commented Mar 5, 2020

@timneutkens is this feature going to be included soon? I have another problem having an index.js that exports the [...slug].js to catch the root index, it's actually creating 2 separated lambdas.

@eddyw
Copy link

eddyw commented Mar 6, 2020

@flybayer @yanv1991 if it's helpful, as a workaround I'm using the experimental rewrittes in next.config.js

modules.export = {
  experimental: {
    rewrites: async () => [
      { source: '/', destination: '/index?slug=index&slug' }
    ]
  }
}

Of course, not quite the same. It'll have /index as pathname instead of / and slug will be ["index"]. Also, using next/link is tricky since it isn't aware of this rewrite 🤷‍♀

For now, it's okay-ish (at least not two lambdas generated) but I'm also waiting to see any progress on this, so I can remove this rewrite and the ugly thing I have to make next/link work with this setup when it needs to go to the root path (with pre-fetching thing).

@programbo
Copy link
Contributor

programbo commented Mar 7, 2020

The description by @Janpot is exactly the behaviour I expected and have been implementing with a now.json rewrite rule that weirdly only works on Now (not with now dev). My use case is to capture all routes (including NO route) and pass them through to an headless CMS.

EDIT:
My new workaround is just to have an additional index.tsx with the following:

import Page from './[...slug]'

export default Page

This works with now dev.

@yanv1991
Copy link

yanv1991 commented Mar 8, 2020

@programbo the problem with that approach is because creates 2 lambdas one for [...slug] and one for index.tsx

@OriginalEXE
Copy link
Contributor

I came here because I ran into the same issue, expecting the Catch all route to catch the root as well.

The only problem I see is that people out there might by now be relying on the current behavior, so if we could handle this in a backward-compatible way, that would be amazin.

@TommySorensen
Copy link

TommySorensen commented Mar 16, 2020

What about this in the pages/index.js file?

export { default, getStaticProps, getStaticPaths } from './[...slug]'

I haven't tested it yet, but it's used in this example

@dpfavand
Copy link

I'm running into this as well, building out static pages with the new 9.3 SSG functions. To me, ideally, this would behave exactly as @Janpot described in the third option in their above comment.

For our architecture, we would basically have a pages/[...path].jsx file that would catch both the index page (if defined as a route from getStaticPaths) as well as any other pages defined by our CMS.

@zackkrida
Copy link
Contributor

zackkrida commented Apr 19, 2020

fwiw I had a client ask me about this the other day. Same approach of doing pages/[...slug].js so it was trivial for us to generate a pages/index.js with duplicated code, but was still something that threw them off for a long time. Here's the basic approach:

// pages/hello/[...world].js
export default function World() {
  return <div>...</div>
}

// Do what you need to fetch data
// pages/hello/index.js
import Index from './[world]'
export default Index

// Do what you need to fetch data, but with hardcoded value

@kylemh
Copy link
Contributor

kylemh commented Apr 24, 2020

I'd also like this! For deployments, I've just been redirects for anybody that goes to /resources (get sent to /resources/1, but basically that route is never NOT paginated.

In local dev it's a bit of a chore to have to remember to add the page number for every route.

@laleksiunas
Copy link

laleksiunas commented May 7, 2020

I would also benefit from this feature. Right now another option seems to be implementing a custom routing which I think is overcomplicated for this case.

@timneutkens What about having another operator before the slug, something like pages/~[...slug].js. It will not break existing applications and it does not look obfuscated to me. I am willing to submit a PR if such an API looks reasonable to you.

@GitSquared
Copy link

GitSquared commented May 14, 2020

From the Dynamic Routing docs:

A good example of catch all routes is the Next.js docs, a single page called pages/docs/[...slug].js takes care of all the docs you're currently looking at.

It seems they use the experimental re-routing feature themselves.

What about having another operator before the slug, something like pages/~[...slug].js

My 2 cents: something like ~path[...asQuery].js.
Example:

pages/
  about/
    index.ts
  ~dashboard[...page].js
  ~blog[...post].js
  index.js

The following would match:

/about       -> pages/about/index
/dashboard*  -> pages/~dashboard with router.query.page populated
/blog*       -> pages/~blog with router.query.post populated
/            -> pages/index

This allows to easily see the difference between catch-all-paths routes and traditional folder-based routing, and allow customizing the name of the router.query variable. A default value for the queries could be index, or perhaps simply undefined.

@timneutkens
Copy link
Member

timneutkens commented May 14, 2020

It seems they use the ugly experimental re-routing hack themselves, lol.

We run experimental options against nextjs.org, vercel.com etc to test them against production workloads. I'd hardly call new features "ugly hacks", it's a new feature being adopted in projects we control.

~ can't be used as a filename character btw, it won't work on all platforms.

Something we've talked about it having [[optionalparams]] syntax for optional routes.

@GitSquared
Copy link

GitSquared commented May 14, 2020

I'd hardly call new features "ugly hacks", it's a new feature being adopted in projects we control.

Apologies, I realize this may have come off more aggressive than I intended it to be. What I meant is that it's undocumented and not production-ready for teams outside Vercel.

Something we've talked about it having [[optionalparams]] syntax for optional routes.

Could you clarify what you mean by "optional routes"?

@timneutkens
Copy link
Member

Could you clarify what you mean by "optional routes"?

Optional route params, the thing that is asked for in the feature request.

@Timer
Copy link
Member

Timer commented May 14, 2020

Concretely, making a page named: pages/hello/[[...world]].js would match /hello, /hello/1, and /hello/2/3, etc.

Today, making a page named: pages/hello/[...world].js only matches /hello/1, and /hello/2/3, etc, but not /hello.


We're happy to take a PR if anyone wants to work on this.

@Janpot
Copy link
Contributor

Janpot commented May 14, 2020

Concretely, making a page named: pages/hello/[[...world]].js would match /hello, /hello/1, and /hello/2/3, etc.

@Timer What should be the value of the world parameter when I access the route /hello? undefined or []?

I assume this syntax would also work for pages/hello/[[world]].js? What would be the value of world in this case? undefined or ''?

Personally I lean towards [] and ''.

@timneutkens
Copy link
Member

Personally I lean towards [] and ''.

Yeah agreed that's probably the best way 👍

@laleksiunas
Copy link

pages/hello/[[...world]].js looks nice! I can start working on this next week, please let me know if someone begins working sooner.

@Timer
Copy link
Member

Timer commented May 14, 2020

For pages/hello/[[...world]].js, accessing /hello would yield an empty array ([]).

We should also throw a build error if you define both pages/hello/[[...world]].js and pages/hello.js or pages/hello/index.js.

Please do not PR support for pages/foo/[[bar]].js at this time, or, make it a separate PR. There's a lot more nuance that goes into this version (route ranking), and is better handled by rewrites. The optional-catch-all can land a lot faster.

@Janpot
Copy link
Contributor

Janpot commented May 14, 2020

@Timer What's the behavior of

/pages
  /hello
    /[[...world]].js
  /index.js

Would it error?

@laleksiunas I just started setting up some tests for it, but I'm not sure yet whether I'm going to finish this, so you can take it if you want.

@Timer
Copy link
Member

Timer commented May 14, 2020

We should also throw a build error if you define both pages/hello/[[...world]].js and pages/hello.js or pages/hello/index.js.

👍

@laleksiunas
Copy link

@laleksiunas I just started setting up some tests for it, but I'm not sure yet whether I'm going to finish this, so you can take it if you want.

@Janpot Well, I wouldn't start working this week, either way. Just ping me if you see that you will not have time to finish it up and I can continue your work.

kodiakhq bot pushed a commit that referenced this issue May 19, 2020
…sh (#10502)

Failing test case for #10488 (comment)

This used to give a 500 in dev environment
chibicode pushed a commit to chibicode/next.js that referenced this issue May 21, 2020
…sh (vercel#10502)

Failing test case for vercel#10488 (comment)

This used to give a 500 in dev environment
@Vadorequest
Copy link
Contributor

Vadorequest commented May 28, 2020

FYI this seems to have been fixed by #10502 and released 8 days ago in v9.4.2

https://github.com/vercel/next.js/releases/tag/v9.4.2

@Timer
Copy link
Member

Timer commented May 28, 2020

@Vadorequest this was fixed by #12887, not #10502.

#10502 is a totally unrelated PR!

@Vadorequest
Copy link
Contributor

Vadorequest commented May 28, 2020

Thanks @Timer, I guess I got confused. (but #10502 doesn't seem unrelated to this issue, since it mentions this issue in its description)

image

@Timer
Copy link
Member

Timer commented May 28, 2020

It originally tried to fix this issue, but turned into something completely different.

Also note that it links to a very specific comment, only talking about 404 vs 500 behavior.

This issue is about asking that index routes 200.

@Darren120
Copy link

The description by @Janpot is exactly the behaviour I expected and have been implementing with a now.json rewrite rule that weirdly only works on Now (not with now dev). My use case is to capture all routes (including NO route) and pass them through to an headless CMS.

EDIT: My new workaround is just to have an additional index.tsx with the following:

import Page from './[...slug]'

export default Page

This works with now dev.

clean solution!

@loicnestler
Copy link

Is it possible that this doesn't affect plain serverless node functions (decoupled from next.js)?
Yes, I know that next.js internally converts everything from the api/ directory to a serverless function, but I'm unable to set-up optional catch-all routes without next.js (using next.js it works fine tho).

Working: ✅ optional catch all example from https://github.com/storyofams/next-api-decorators/tree/master/examples/route-matching
Not working: 🚫 same example & same code but as a plain serverless function without next.js.

@balazsorban44
Copy link
Member

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 vercel locked as resolved and limited conversation to collaborators Jan 27, 2022
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 a pull request may close this issue.