vercel / next.js

The React Framework
https://nextjs.org
MIT License
127.13k stars 27.02k forks source link

Dynamic Modal Interception is Greedy #70651

Open khuezy opened 1 month ago

khuezy commented 1 month ago

Link to the code that reproduces this issue

https://github.com/khuezy/next-modal-bug

To Reproduce

  1. npm install
  2. npm run dev
  3. Go to localhost:3000/gallery
  4. Click on /gallery/dynamic
  5. See dynamic route modal interception
  6. Go back
  7. Click /gallery/static
  8. See that the static route is intercepted when it shouldn't

Current vs. Expected behavior

Modal Interception intercepts static routes as dynamic.

/app
  /gallery
    /[dynamic]
      page.tsx
    /static
      page.tsx
    /@modal
      /[dynamic]
        page.tsx

When linking to /gallery/static, the modal interceptor intercepts it as /gallery/[dynamic].

Without "@modal", the /gallery/static is processed before the /gallery/[dynamic]route. But with "@modal" interception, it is intercepting the /gallery/static path as [dynamic]

So with that, the expectation of "@modal" route interception is the same. "@modal" should not intercept static routes.

Provide environment information

Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 23.0.0: Fri Sep 15 14:41:43 PDT 2023; root:xnu-10002.1.13~1/RELEASE_ARM64_T6000
  Available memory (MB): 32768
  Available CPU cores: 10
Binaries:
  Node: 22.6.0
  npm: 10.8.2
  Yarn: N/A
  pnpm: 9.7.0
Relevant Packages:
  next: 15.0.0-canary.173 // Latest available version is detected (15.0.0-canary.173).
  eslint-config-next: N/A
  react: 18.3.1
  react-dom: 18.3.1
  typescript: 5.6.2
Next.js Config:
  output: N/A

Which area(s) are affected? (Select all that apply)

Parallel & Intercepting Routes

Which stage(s) are affected? (Select all that apply)

next dev (local)

Additional context

No response

abhi12299 commented 1 month ago

Does this only happen during next dev?

Update: this also happens on next start.

abhi12299 commented 1 month ago

Based on my investigation, here's what I found:

  1. The route matcher for interception routes takes precedence over checking for exact matches in the file system. This can be found here: https://github.com/vercel/next.js/blob/1d1b846a89f1316ad190be97bd472db61d4f03e0/packages/next/src/server/lib/router-utils/resolve-routes.ts#L67-L97

fsChecker.rewrites.beforeFiles is the route matcher for interception routes.

  1. This means whenever a request comes for /gallery/static, it is converted to /gallery/(.)static which matches with the interception route pattern of /gallery/(.)[dynamic].
  2. When I moved fsChecker.rewrites.beforeFiles after the check_fs matcher, it fixed this issue but introduces another major issue - interception routes are no longer given priority over exact fs matches, meaning that if we want to intercept /gallery/a and if that route exists as a static route, it will no longer be intercepted.

This means that we have to do the following:

  1. Identify if the interception is made on a dynamic path parameter.
  2. If the matching is successful, rather than greedily resolving this match, we continue down the routes and check if an exact match exists for that path param on the same route segment. For example, if we intercept on /gallery/(.)[dynamic], we have to check for an exact match on /gallery/<path>. Similarly, if we intercept on /gallery/nested/(.)[dynamic], we have to check for an exact match on /gallery/nested/<path>. During this process, we have to ensure that we do not match with /gallery/[dynamic] or /gallery/nested/[dynamic] - we have to look for an exact match in the fs. We also have to take care of the (...) interception pattern.
  3. If we find a match, we return that exact match's route and skip interception.

This process introduces overhead during the route resolution step, and is also not straightforward to implement.

Given the current implementation, it is clear that interception routes are given priority over anything else, which makes sense, so this behaviour can be documented and we can explicitly mention in the docs that if the user wants to intercept a dynamic path parameter, we will perform greedy matching.

khuezy commented 1 month ago

Thanks for your time investigating! If this isn't supported, then I can just move the dynamic route down a subpath.

abhi12299 commented 1 month ago

I'm not a core contributor, so let's wait for someone from the core team to look at this issue. Let's see what happens.

viniciuspalma commented 1 month ago

I have these two pages:

  1. src/app/@modal/(.)writer/drafts/[draft_id]/chapters/[chapter_id]/page.tsx
  2. src/app/writer/drafts/[draft_id]/chapters/[chapter_id]/page.tsx What would make the modal appear once clicking any link on the client-side. But instead it doesn't work. I tested on all the other subsequent routes and works:

Moving the page to src/app/@modal/(.)writer/drafts/[draft_id]/chapters/page.tsx Captures the transition and renders the modal.

As well on:

src/app/@modal/(.)writer/drafts/[draft_id]/page.tsx
src/app/@modal/(.)writer/drafts/page.tsx
src/app/@modal/(.)writer/page.tsx

All these examples works perfectly but as soon I add the page on the second dynamic route containing the "chapter_id" it stop to work.

There's any limitation on the API as how far nested the intercept would work or I'm doing something wrong?