vercel / next.js

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

[NEXT-1160] Clicking Links in intercepted routes does not unmount the interceptor route #49662

Closed mkarajohn closed 11 months ago

mkarajohn commented 1 year ago

Verify canary release

Provide environment information

Operating System:
      Platform: linux
      Arch: x64
      Version: #41~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Fri Mar 31 16:00:14 UTC 2
    Binaries:
      Node: 18.12.1
      npm: 8.19.2
      Yarn: 1.22.19
      pnpm: N/A
    Relevant packages:
      next: 13.4.2-canary.5
      eslint-config-next: N/A
      react: 18.2.0
      react-dom: 18.2.0
      typescript: N/A

Which area(s) of Next.js are affected? (leave empty if unsure)

App directory (appDir: true), Routing (next/router, next/navigation, next/link)

Link to the code that reproduces this issue

https://codesandbox.io/p/github/mkarajohn/nextgram/draft/restless-leftpad?file=/components/frame/index.js

To Reproduce

  1. Open the codesandbox in a new separate tab in order to be able to see the actual browser URL
  2. Click on a photo in order to intercept the route and open the modal with the photo details.
  3. Instead of clicking on the overlay click the "go back" link instead, located just below the photo

Describe the Bug

When you click the Link and the URL changes to / the interceptor route remains mounted and does not go away.

Expected Behavior

I would expect that since the route was changed (via Link no less) the interceptor page should disappear once the route that it was intercepting was changed.

Screencast from 11-05-2023 04:23:04 ΜΜ.webm

Which browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

NEXT-1160

IvanRomanovski commented 1 year ago

@SpBills thanks a lot for explaining what I was trying to say in such great detail. Your explanation is awesome! 😀

I also agree that it's not great DX and it would be great if one could specify the desired behaviour for the slot. The default option imho should be that slot is always cleared when navigating away from intercepted route, but I see how current behaviour can be beneficial in some cases. For example, if I would want to integrate chat dialog (in bottom right corner of my site - think Facebook) which is persistent across routes but also has a dedicated route for full screen view.

hsuanyi-chou commented 1 year ago

does intercepting route work with basePath?

hoping there's a workaround, even if it's a bit tricky.

https://github.com/vercel/next.js/issues/52624

lira1705 commented 1 year ago

Okay, I have done some significant work here to make this work for me. Here is the context:

I will do everything in my power to stay away from client-side state management. I love using routes to manage all of my locational state (including modals). I noticed that whenever I was doing this, redirect("/profile") did not redirect me back! Instead, my modals stayed open.

Using hints from above, I discovered that what is really important is the layout on which your modal is being rendered. That is, you should be rendering your slot in the layout that minimally contains your intercept routes.

So, if I have a route structure like such:

/profile
/profile/group/[id]/edit // A modal.
/profile/group/[id]/delete // A modal.

I really only want to be slotting in the minimal containing folder. Here, that would be group. Let's discuss why we want this.

Why Only Grab the Minimum?

Let's start by describing our file structure. Here, I lay out what we do not want.

/profile
/profile/layout.tsx
/profile/@dialog/(.)group/[id]/edit 
/profile/@dialog/(.)group/[id]/delete

The layout.tsx here looks something like...

import { ReactNode } from "react";

export default function Layout({
  children,
  dialog,
}: {
  children: ReactNode;
  dialog: ReactNode;
}) {
  return (
    <>
      {children}
      {dialog}
    </>
  );
}

Now, if I am in the edit modal and I redirect back to /profile, am I changing the layout properties? No. I might argue that I should be, but right now you are not. Instead, you are just saying "rerender the layout in the state you already have", which still contains your slot.

How to grab the minimum

I'll keep the answer simple: we want to use route groups.

/profile
/profile/layout.tsx
/profile/(modal)/layout.tsx
/profile/(modal)/@dialog/(.)group/[id]/edit 
/profile/(modal)/@dialog/(.)group/[id]/delete

There are some nuances here. First, remember that we do not want the profile/layout.tsx to be rendering the @dialog slot. Do not do that. Instead, use the (modal)/layout.tsx to render your dialog slot. Second, do not use (modal)/layout.tsx to render children either. Only use it to render your dialog slot.

// /profile/layout.tsx
import { ReactNode } from "react";

export default function Layout({
  children,
}: {
  children: ReactNode;
}) {
  return (
    <>
      {children}
    </>
  );
}
// (modal)/layout.tsx
import { ReactNode } from "react";

export default function Layout({
  dialog,
}: {
  dialog: ReactNode;
}) {
  return (
    <>
      {dialog}
    </>
  );
}

Summary

Like @IvanRomanovski said- this is likely not a bug, we're just not using the system right. Now, that's arguably a bad take from a DX perspective and I do think that this should be changed. But hopefully this sheds some light onto the complexities behind this.

That was veeeeery informative. Thank you very much, but i have a few questions.

That solution fix the navigation problem, but i don't think that will fix the main problem. Since im rendering either the children or the slot, when the route is intercepted only my slot will be rendered and my content from the rootLayout will disappear. This isn't the behaviour im looking for. In the docs the slot should be used to render both the children AND the modal. How can i achieve that using your solution?

SpBills commented 1 year ago

Not at my computer right now...

So, because you're going into a nested layout, you are still rendering both {children} and {modal}. It's just in a nested layout.

For my purposes (absolutely overlapping the page with an element) this is fine. I can see where it's a problem where you want something in a specific part of the outer layout though...

lira1705 commented 1 year ago

Just to clarify, I'm very grateful for your comment. Sorry if my quote seemed too assertive.

zhllucky commented 12 months ago

so,when can the official solve this problem? This problem has caused great trouble

TWolczanski commented 11 months ago

IMHO the cleanest workaround is to create a layout inside the slot and conditionally render children based on the output of the usePathname hook. If you have the following folder structure:

Screenshot from 2023-11-04 22:11:16

You can create the following layout inside @modal:

"use client";

import {usePathname} from "next/navigation";

export default function Layout({children}) {
  const pathname = usePathname();
  return pathname.startsWith("/posts/") ? children : null;
}

If you have multiple intercepting routes this seems to be less messy than rendering conditionally inside the parent layout (especially if the parent layout is the root layout, which can't be made a client component).

The cleanest solution would obviously be using catch-all routes, but they're still broken.

wlechowicz commented 11 months ago

This is still an issue between 13.5.6 and 14.0.2-canary.13. The catchAll method of dismissing the modal (the intercepted route), described in docs, doesn't work. Neither does router.push() or a Link that leads to a different, non-intercepted route.

Workarounds involving duplicating the pages inside the intercepted route tree don't work if your page structure involves nested segments etc.

@TWolczanski you don't have to put that in the layout - in my case, there is no separate layout to handle this, because modal is nested in the root layout. Techincally you could turn the root layout into client comp and usePathname there, but that's overkill. Instead, I put this in the Modal component itself:

  const pathname = usePathname();
  if (!pathname.startsWith("/movie/")) {
    return null;
  }

I think locating this in the Modal, as opposed to a layout, makes it harder to be forgotten about once a proper fix is actually in place.

github-actions[bot] commented 11 months ago

This closed issue has been automatically locked because it had no new activity for 2 weeks. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.