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

[v6] Nested routers #7375

Closed
rovansteen opened this issue May 25, 2020 · 15 comments
Closed

[v6] Nested routers #7375

rovansteen opened this issue May 25, 2020 · 15 comments

Comments

@rovansteen
Copy link

Hi, currently in v6 it's not possible to have supported routers because of this check. In the past it was possible to have a <MemoryRouter /> inside of a <BrowserRouter />. This is very useful if you have a section with multiple routes that can work standalone but can also work within a different page in a modal. In that case you can wrap it in a memory router and it the routing still works without navigating away from the current page.

Would you consider a PR that lifted this restriction or provide some workaround for this?

@timdorr
Copy link
Member

timdorr commented May 25, 2020

Unfortunately, because of the way context is being used to allow for nested routes, we have to drop support for nested routers. It's not as simple as removing that one check, so we can't easily add it back. If you can make a PR that fully tests nested routers and resolves any issues with them, feel free. But right now it's not something we're considering doing ourselves.

@eps1lon
Copy link
Contributor

eps1lon commented Nov 22, 2021

Unfortunately, because of the way context is being used to allow for nested routes

@timdorr Could you point to implementation details that prevent nesting router contexts? This is quite the surprising behavior since React context nesting should "just work".

I understand problems with nesting for certain implementations (e.g. BrowserRouter). But BrowserRouter > MemoryRouter seems harmless to me (maybe even MemoryRouter > MemoryRouter). Would help alot to understand what we should test about nesting routers.

@MeiKatz
Copy link
Contributor

MeiKatz commented Nov 22, 2021

There might be a dirty hack to achieve this feature: RRv6 exports the LocationContext as UNSAFE_LocationContext. The LocationContext is used in the hook useInRouterContext() where the value of LocationContext is checked (should not be null). Therefore you can wrap your MemoryRouter in a LocationContext and set its value to null.

<UNSAFE_LocationContext.Provider value={ null }>
  <MemoryRouter>
    { /* some routes not related to the outer router */ }
  </MemoryRouter>
</UNSAFE_LocationContext.Provider>

@brianbolnick
Copy link

following - would love to see some implementation that supports this again in v6.

@KutnerUri
Copy link
Contributor

KutnerUri commented Dec 13, 2021

yes, UNSAFE_LocationContext.Provider works, not perfectly though.
--> for example, I clicked on a link to="target", and the memory location changed to be "/docs/target" (instead of the expected (/target)

It seems like MemoryRouter can just reset all the contexts, and would work as if it was the first in the page, unless it's keeping some global state.
Can you list all the relevant contexts?

@KutnerUri
Copy link
Contributor

KutnerUri commented Dec 13, 2021

Of course, this would lead users to unexpected behaviors, and should be discouraged.
How about requiring an explicit flag, like:

<MemoryRouter UNSAFE_allowNesting />

(separate comment so people could vote on it separately)

@tylergraf
Copy link

Has anyone figured out a workaround or alternative for this? We use MemoryRouters all over inside BrowserRouters for flows inside of overlays.

@remix-run remix-run deleted a comment from fortesp Sep 21, 2023
@jamesmarrs
Copy link

jamesmarrs commented Nov 2, 2023

@tylergraf I only use the react-router-dom package strictly for routing, I dont use any of the added data abstractions.. so your success with this implementation could be different than mine..

When we were upgrading our projects, we had lots of components that SHOULD NOT update browser history, and were built originally with a <MemoryRouter /> to be self contained. An example for us is a <Comments {...} /> sub panel. Our tab structure on this page was already hooked up to browsing history, and made a comment delete modal very cumbersome to hook up since you had to opt in to one <BrowserRouter />.

Since we already had all these great <Modal />, and <Tab /> components built on top of the react-router-dom API that work exclusively with a browser and memory routers, we werent going to re-invent the wheel and start creating our own "memory stack" when react-router-dom does this perfectly.

Heres an example of a <MemoryRouter /> inside of a <BrowserRouter />:

URL: /tab-1
Screenshot 2023-11-02 at 2 25 44 PM

Example of a self contained modal within the <Comments /> sub panel, not adjusting browser history and still retaining its current view in the background using the <MemoryRouter />

URL: /tab-1
Screenshot 2023-11-02 at 2 25 59 PM

You have a few options here...

  1. you can rebuild your own memory router, modal logic, tab logic, but this is time consuming and react-router-dom does this perfectly already.
  2. You can try and rebuild all your complex logic to adapt one browser router. We found this to be cumbersome since you can technically trigger a comment delete on any URL.

Having one top level <BrowserRouter /> with <MemoryRouter />s within it currently works fine for us by removing this bit of code:

invariant(
    !useInRouterContext(),
    `You cannot render a <Router> inside another <Router>.` +
    ` You never need more than one.`
);

The CONS:

You need to know what router context your currently in, and for us that really wasn't an issue.

If you are trying to change browser history within your sub router, you can export your browser router... and do something like this:

export const router = createBrowserRouter(createRoutesFromElements(...))
and just call router.navigate()

I am quite sad that theres no plan to add this back in, as i believe this feature is needed to create very complex and reusable UIs without re-inventing how to show modals, tabs, etc when react-router-dom does this so well...

@paralin
Copy link

paralin commented Nov 7, 2023

@jamesmarrs I agree: removing that invariant() line makes it work perfectly; whichever Router context you're within currently is the one the React components use. This makes logical sense and should be possible with this package. Just add a note in the docs saying that these components will use whatever the closest parent RouterContext is.

paralin added a commit to aperturerobotics/remix-react-router that referenced this issue Dec 19, 2023
remix-run#7375 (comment)

Signed-off-by: Christian Stewart <christian@aperture.us>
@paralin
Copy link

paralin commented Dec 19, 2023

@jamesmarrs I ended up forking react-router to accomplish this, wish it wasn't necessary, but it seems like no progress has been made on #9601 - https://github.com/aperturerobotics/remix-react-router

paralin added a commit to aperturerobotics/remix-react-router that referenced this issue Dec 20, 2023
remix-run#7375 (comment)

Signed-off-by: Christian Stewart <christian@aperture.us>
@tejasviSri
Copy link

@jamesmarrs I ended up forking react-router to accomplish this, wish it wasn't necessary, but it seems like no progress has been made on #9601 - https://github.com/aperturerobotics/remix-react-router

@paralin , do you've a working example using your forked repo? I used your forked package it still doesn't differentiate between parent nav and child nav.

@paralin
Copy link

paralin commented Jan 28, 2024

@tejasviSri you can nest Router in that version and since the router stuff uses RouterContext, whichever parent context is there is used. But feel free to open an issue there with your use case and I'll check.

@teameh
Copy link
Contributor

teameh commented Jan 29, 2024

I think you've meant to mention @tylergraf?

@paralin
Copy link

paralin commented Jan 29, 2024

@teameh mentioned you instead of @tejasviSri - apologies

@teameh
Copy link
Contributor

teameh commented Jan 29, 2024

No problem :D

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

No branches or pull requests