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

Enable ability to have nested MemoryRouters #9112

Conversation

auaustorg-ms
Copy link

@auaustorg-ms auaustorg-ms commented Jul 29, 2022

Hello everyone!
My name is Austin and I am a dev on Xbox. I primarily work on Xbox Cloud Gaming. We currently leverage the react-router package and enjoy it.

The Problem

We are looking to upgrade to v6 soon and at the same time we are doing some work around Modals and wanted to leverage a MemoryRouter to control a Modal stack. We thought it would be great to navigate our Modals similar to how we navigate the rest of the site.

Key features include

  • navigate between modals while passing data
  • pop a modal
  • push a modal
  • clear the stack
  • animated transitions, etc

Our desired features led us back to react-router. We did not want to re-invent the wheel and thought something like a MemoryRouter would be perfect because we wanted our Modal routing system to not be exposed to the Browser. You cannot deep link into a modal or anything. It is just meant to be used for in-memory navigation, exactly the purpose of <MemoryRouter>.

So with that in mind, we went looking around to answer the question "Can you have a MemoryRouter inside of a BrowserRouter?". We ended up finding issues like the following:
#9109
#8817
#7375

The Solution

We realized it is not possible to nest Routers in v6 and it was a bummer. After looking through the react-router code, we came to the conclusion that is is definitely possible to enable having nested MemoryRouter's inside of other Routers. So here we are, with a PR that enables nesting MemoryRouter's.

Example Usage:

import {
  BrowserRouter,
  Routes,
  Route,
  Outlet,
  Link,
  createScopedMemoryRouterEnvironment,
} from "react-router-dom";

const {
  MemoryRouter: ScopedMemoryRouter,
  Routes: ScopedRoutes,
  Route: ScopedRoute,
  useNavigate: useScopedNavigate,
  Link: ScopedLink,
} = createScopedMemoryRouterEnvironment();

function ModalRouter() {
  return (
    <ScopedMemoryRouter initialEntries={["/"]}>
      <ScopedRoutes>
        <ScopedRoute path="/" element={<FirstModal />} />
        <ScopedRoute path="second" element={<SecondModal />} />
        <ScopedRoute path="third" element={<ThirdModal />} />
      </ScopedRoutes>
    </ScopedMemoryRouter>
  );
}

function SecondModal() {
  const navigate = useScopedNavigate();
  return (
    <div className="baseModal">
      <div className="modalContents">
        <h1>Second Modal</h1>
        <ScopedLink to="/third">Go to third modal</ScopedLink>
        <button onClick={() => navigate(-1)}>Go back</button>
        <Link to="/">Go home</Link>
      </div>
    </div>
  );
}

function App() {
  return (
    <BrowserRouter>
      <Routes>
        <Route path="/" element={<Layout />}>
          <Route index element={<Home />} />
          <Route path="about" element={<About />} />
          <Route path="dashboard" element={<Dashboard />} />
          <Route path="modals" element={<ModalRouter />} />
          <Route path="*" element={<NoMatch />} />
        </Route>
      </Routes>
    </div>
  );
}

You can see this example under the new examples/scoped-memory-router demo.

How it works

There is a new method exported called createScopedMemoryRouterEnvironment which returns the various components / hooks needed for a scoped memory router.

This function does not return things like BrowserRouter since it does not make sense to have nested BrowserRouter's.
It works by making almost all components / hooks into create methods. Instead of having useLocation use the default context, we replaced it with a createLocationHook that is passed a context. We then use the default context for the default useLocation export, but it then allows us to create a new useLocation hook inside of createScopedMemoryRouterEnvironment.

This creator pattern was taken from react-redux as seen in their useSelector implementation.

We've leveraged react-redux createSelectorHook before and had a great time working with that pattern. We believe this pattern can also be used in react-router and have proved it out in this PR. These proposed changes would resolve issues like:
#9109
#8817
#7375

Notes For Reviewers

This is my first time contributing to a large open source project, so please bear🐻 with me. I still need to add more test cases, but wanted to get this review up to gather feedback early. I am open to any and all changes, including naming. As we know naming is one of the hardest things in CS 😜. I am not super familiar with all the various routers like the new DataRouter's, etc. I am not sure exactly what routers are allowed to be nested. Should DataMemoryRouter be nested? These are questions I am hoping you can answer. If you are open to these contributions, then I can write docs for this new feature, but I did not want to get ahead of myself.

FAQ

Why do you need to create unique contexts?

The implementation in this PR allows navigation of different routers from any level of the application by allowing MemoryRouters to have distinct contexts.

The createScopedMemoryRouterEnvironment call returns scoped hooks that allow you to perform actions within the scoped MemoryRouter. You can see this at play in the example here:

const {
  MemoryRouter: ScopedMemoryRouter,
  Routes: ScopedRoutes,
  Route: ScopedRoute,
  useNavigate: useScopedNavigate,
  Link: ScopedLink,
} = createScopedMemoryRouterEnvironment();

If you'd like to navigate the root BrowserRouter you'd use the regularly imported hooks from the lib and if you're navigating within the scoped memory router I'd anticipate you'd run this in a module in user-land code and re-export out these generated values, i.e.:

src/embedded-application/scoped-react-router.ts

export const {
  MemoryRouter,
  Routes,
  Route,
  useNavigate,
  Link,
} = createScopedMemoryRouterEnvironment();

src/embedded-application/component/Foo.tsx

import { useNavigate as useScopedNavigate } from '../scoped-react-router';
import { useNavigate } from 'react-router';

This allows us to navigate within the scoped Memory Router, but also navigate at the Browser Router level if needed.
Cheers!

@changeset-bot
Copy link

changeset-bot bot commented Jul 29, 2022

⚠️ No Changeset found

Latest commit: db89bc7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jul 29, 2022

Hi @auaustorg-ms,

Welcome, and thank you for contributing to React Router!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jul 29, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@auaustorg-ms
Copy link
Author

Tagging a few folks who look like can review PRs. This PR has been up for about 2 weeks and I was hoping to get some feedback. Thank you!

@chaance @brophdawg11 @timdorr @ryanflorence

@timdorr
Copy link
Member

timdorr commented Aug 12, 2022

I'll be honest: I don't think this is going to be merged in. At a high level, you're creating scopes for Context values, which is not really how Context is meant to be used. Instead, you should simply re-use the same Provider with a new value and now you have a new scope. createScopedMemoryRouterEnvironment should be entirely unnecessary.

Instead, you should just be able to nest a MemoryRouter, no questions asked. I think a far more likely-to-be-merged PR would be one that fixes the bugs and issues (mostly around nesting) that occur when you do that. There should ideally be no API changes.

Sorry to be a Negative Nancy, but I'd rather be realistic here. I do appreciate the hard work that went into this PR. I think we can take some of the intent here and turn it into something more likely to be merged.

@auaustorg-ms
Copy link
Author

I'll be honest: I don't think this is going to be merged in. At a high level, you're creating scopes for Context values, which is not really how Context is meant to be used. Instead, you should simply re-use the same Provider with a new value and now you have a new scope. createScopedMemoryRouterEnvironment should be entirely unnecessary.

Instead, you should just be able to nest a MemoryRouter, no questions asked. I think a far more likely-to-be-merged PR would be one that fixes the bugs and issues (mostly around nesting) that occur when you do that. There should ideally be no API changes.

Sorry to be a Negative Nancy, but I'd rather be realistic here. I do appreciate the hard work that went into this PR. I think we can take some of the intent here and turn it into something more likely to be merged.

Hey @timdorr, thank you for the response. I wanted to clarify why I had to add createScopedMemoryRouterEnvironment. You are correct in a sense that we could re-use the same Provider with a new value, but that then limits us to only ever navigating within the nested MemoryRouter.

There are use cases where the flow might be like the following:

BrowserRouter -> HomePage -> [MemoryRouter -> Modal1 -> Modal2] -> SettingsPage

Where you start on the HomePage, get shown Modal1 and Modal2 (both modals inside a memory router) and on Modal2 there is a button to take you to a SettingsPage which is now back in the BrowserRouter context.

If Modal2 is like:

function Modal2() {
  const navigate = useNavigate();
  
  return <button onClick={() => navigate(...)}>Go to Settings Page</button>
}

Clicking the button would do nothing because useNavigate would be tied to the MemoryRouter and now you are stuck. Because createScopedMemoryRouterEnvironment returns hooks as well, we can unlock the required functionality.

const {
  MemoryRouter: ScopedMemoryRouter,
  useNavigate: useScopedNavigate,
} = createScopedMemoryRouterEnvironment();

// ... use ScopedMemoryRouter inside of a <Route>, etc

function Modal2() {
  const navigate = useNavigate();
  const scopedNavigate = useScopedNavigate();
  
  return <>
    <button onClick={() => navigate(...)}>Go to Settings</button>
    <button onClick={() => scopedNavigate(...)}>Go to Modal 3</button>
  <>
}

This is why we need createScopedMemoryRouterEnvironment. If there is a better solution to this issue, I am open to making the necessary changes. Please let me know your thoughts on this. Thank you so much 😃

@auaustorg-ms
Copy link
Author

@timdorr if you want to chat more about this on the Remix discord or some other medium let me know. Would love to work and contribute towards a solution.

@ElliotChong-MS
Copy link

ElliotChong-MS commented Aug 31, 2022

Hey @timdorr, just wanted to ping on this one since it's been a few weeks. We'd love to help improve react-router all-up and think this could solve the common issue of wanting a nested memory router inside of the broader browser router. Let us know if we can help provide more clarification on why a pattern like createScopedMemoryRouterEnvironment is necessary.

@timdorr
Copy link
Member

timdorr commented Aug 31, 2022

Again, I think this is overcomplicating the issue.

The problems with nested hooks like useNavigate or useHref can be resolved by setting basename on your nested MemoryRouters. One thing we could do there is compute that for you automatically if a Router is nested within another Router. But that's just an optimization; you can pass down that value on your own.

I guess I'm not understanding why there is a need for another new instance of a Router nested inside your application. Relative paths and absolute paths should be more than sufficient to navigate within a section of your overall app. You can nest Routes, so you can establish new sub-paths for these sections. As long as you link and navigate relatively, you can keep within the section. And when you need to go elsewhere, you can use an absolute link/navigate.

@ElliotChong-MS
Copy link

Thanks for the response @timdorr! Apologies if we haven't done the best job of explaining what problem space this is addressing.

Imagine you have a "normal" web application that has routes driven by the browser's address bar. The routes in this app could be things like a home page, user profile page, other content pages, etc. Existing react-router patterns apply here (including basename as you called out) and everything works great.

Now imagine you'd also like to have a Settings panel inside of this application that can be invoked from any route on the website and sits on top of the app like a modal. Interactions and navigations within this panel are isolated to the panel and don't impact the broader application / browser's URL. This Settings panel has multiple screens within it (such as Account, Video, Audio, Accessibility, etc.) that can link to one another and be directly linked to from various parts of the larger application. Additionally, these screens may also have "L2s" or sub-screens that dive deeper into the panel... If a top-level route in the Settings panel might be defined as /accessibility then you could also have a child page at say /accessibility/colors. In addition to having this sub-screen, you'd like to be able to make use of the history of this Settings application and be able to navigate back to where you came from via a back button directly shown on the top-left of these screens.

This is all to illustrate that if you're ever trying to build a route-driven application that is embedded within a broader "typical" application it would be a perfect use-case to have a nested MemoryRouter so that you could have all of the benefits of the react-router library inside of the embedded app. This PR also leaves the door open for the embedded applications to still navigate the top-level app using the default react-router hooks if desired but also keeps the navigations isolated to the embedded application by utilizing scoped hooks created by the scopedMemoryRouterEnvironment call.

The highest level use-case I can sum this up as is whenever you have overlays and they're serious enough to benefit from their own router / history stack this would be a really welcome feature to utilize. We've run into three use-cases in our own website that would benefit from this pattern (the Settings panel mentioned above, a fly-out "Social" panel that a user can invoke anywhere on the site and it has numerous routes and sub-routes inside of it, and a general pattern for modals and being able to easily route to a specific one or supporting sub-states).

Hopefully this helps frame the problem we're attempting to solve here. Are there other alternatives you see to our problem that we might have missed? Any further questions or context I can help provide?

@auaustorg-ms
Copy link
Author

@timdorr Friendly ping. Any thoughts on @ElliotChong-MS comment? Thank you.

@ryanflorence
Copy link
Member

I'm interested in this conversation but am a bit busy with the 6.4 release. I've only skimmed a few comments.

Just FYI: you can create a brand new React Tree inside of an effect to accomplish what you're after (don't use a portal, that'll share context, the point is to get rid of it).

let ref = useRef();
useEffect(() => {
  ReactDOM.render(<MemoryRouter />, ref.current);
}, []);
return <div ref={ref}/>

Or something like that ...

My big question about these scoped router trees is how do you link back out to the browser router inside the context of a scoped router?

Like Tim, it's also unclear to me why you can't just add some more normal routes?

Again, I haven't had time to read all of the comments, but just wanted to toss out that quick solution to your problem that (should?) work today and a couple questions to think about.

@ElliotChong-MS
Copy link

ElliotChong-MS commented Sep 8, 2022

Hey @ryanflorence, thanks for the comment!

Interesting solution with embedding a new React tree, but I think that would have the issue of being unable to navigate the root BrowserRouter from within the nested React tree's MemoryRouter.

Re: your question on the different interactions: The createScopedMemoryRouterEnvironment call returns scoped hooks that allow you to perform actions within the scoped MemoryRouter. You can see this at play in the example here:

const {
  MemoryRouter: ScopedMemoryRouter,
  Routes: ScopedRoutes,
  Route: ScopedRoute,
  useNavigate: useScopedNavigate,
  Link: ScopedLink,
} = createScopedMemoryRouterEnvironment();

If you'd like to navigate the root BrowserRouter you'd use the regularly imported hooks from the lib and if you're navigating within the scoped memory router I'd anticipate you'd run this in a module in user-land code and re-export out these generated values, i.e.:

src/embedded-application/scoped-react-router.ts

export const {
  MemoryRouter,
  Routes,
  Route,
  useNavigate,
  Link,
} = createScopedMemoryRouterEnvironment();

src/embedded-application/component/Foo.tsx

import { useNavigate } from '../scoped-react-router';

...

The reason we don't want to add normal routes is that the UX we're controlling is global and not tied to the route (it could appear on top of any page / route) and also it shouldn't really impact the browser's URI, it's truly in-memory routing.

I've tried to frame the context there in the last comment with a few real-world use-cases we're running into but if they're not coming across that clearly let me know and I'll take another swipe at it. Thanks a lot for your suggestions and questions, let me know if there is any other context we can provide!

@piecyk
Copy link

piecyk commented Sep 9, 2022

Interesting solution with embedding a new React tree, but I think that would have the issue of being unable to navigate the root BrowserRouter from within the nested React tree's MemoryRouter.

One option is to pass the browser navigate as it's own context, something like https://stackblitz.com/edit/github-8cynrp-ekm7kv click on Modal link, or change route to /modal

But yeah, great idea @ryanflorence with new React Tree 👏

@cascornelissen
Copy link

We're running into this issue with a similar use-case where we're using a single "global" BrowserRouter for the main app navigation and several MemoryRouters inside modals/panes/overlays. This worked amazingly well in react-router v5.

I've tried out the approach mentioned in #9112 (comment) of rendering a new React Tree but we're also using other contexts such as react-redux's Provider which are of course also lost when creating a new tree. So this is not really a workable solution for us.

Just commenting to make it apparent that there are more parties struggling with this and following any progress on this ✌🏼

@piecyk
Copy link

piecyk commented Sep 12, 2022

On other hand why we need new tree? React context can stack, so basic passing MemoryRouter should override already provided contexts. Now we can reset BrowserRouter using UNSAFE_LocationContext, UNSAFE_RouteContext or am i missing something 🤔 https://stackblitz.com/edit/github-8cynrp-aiwgho?file=src%2FApp.tsx

@ansonlouis
Copy link

I'm looking into the option to stack MemoryRouters as well. It seems to me from reading through the thread that a lot of the pushback is around the need to link back to parent routers. In these very complex cases, I feel @piecyk suggestion of creating your own contexts makes sense here, rather than trying to bake things into react-router itself.

@ElliotChong-MS @auaustorg-ms Have you thought of changing to use HistoryRouter along with your own internal MemoryHistory instances and creating the contexts yourself? That should theoretically be quite easy and I don't see why that wouldn't accomplish the goal (it seems to do what I need from some very quick tests). My use case is very similar to yours, but perhaps even more complex, where we have 3 different memory-based routers for different parts of the page, as well as the top-level browser router. Maybe I'm missing something there though...

@auaustorg-ms
Copy link
Author

I'm looking into the option to stack MemoryRouters as well. It seems to me from reading through the thread that a lot of the pushback is around the need to link back to parent routers. In these very complex cases, I feel @piecyk suggestion of creating your own contexts makes sense here, rather than trying to bake things into react-router itself.

@ElliotChong-MS @auaustorg-ms Have you thought of changing to use HistoryRouter along with your own internal MemoryHistory instances and creating the contexts yourself? That should theoretically be quite easy and I don't see why that wouldn't accomplish the goal (it seems to do what I need from some very quick tests). My use case is very similar to yours, but perhaps even more complex, where we have 3 different memory-based routers for different parts of the page, as well as the top-level browser router. Maybe I'm missing something there though...

@ansonlouis How do you handle hooks correctly? Ideally you can use a useNavigate to navigate within a nested memory router OR to navigate at the BrowserRouter level.

@ansonlouis
Copy link

@auaustorg-ms sorry, I actually realized after more testing that react-router simply throws an error if you try to nest HistoryRouters, even if the underlying history object is a MemoryHistory. So, it won't work for nesting. I think @timdorr response here gives hope that it's possible to fix, they just aren't going to do it themselves. TBH, I'm really not sure what these bugs are and why they happen.

However, to answer your question from a theoretical POV, you wouldn't be able to use the core react-router hooks in every situation. useNavigate, for example, would always point to your nearest router, which may/may not be a good thing for your implementation.

Since there can naturally only be one BrowserRouter (unlike Memory/History routers), you'd need to create your own custom hooks that return the BrowserRouter context specifically.

It seems like a lot of work, but it's really not. And, for a non-typical use-case such as nested routers, I think this seems reasonable.

@auaustorg-ms
Copy link
Author

@auaustorg-ms sorry, I actually realized after more testing that react-router simply throws an error if you try to nest HistoryRouters, even if the underlying history object is a MemoryHistory. So, it won't work for nesting. I think @timdorr response here gives hope that it's possible to fix, they just aren't going to do it themselves. TBH, I'm really not sure what these bugs are and why they happen.

However, to answer your question from a theoretical POV, you wouldn't be able to use the core react-router hooks in every situation. useNavigate, for example, would always point to your nearest router, which may/may not be a good thing for your implementation.

Since there can naturally only be one BrowserRouter (unlike Memory/History routers), you'd need to create your own custom hooks that return the BrowserRouter context specifically.

It seems like a lot of work, but it's really not. And, for a non-typical use-case such as nested routers, I think this seems reasonable.

@ansonlouis I agree. This PR allows you to use hooks like useNavigate freely and allows you to have nested routers. This PR creates the custom hooks (tied to the correct context) for you, instead of you need to do it manually.

@ansonlouis
Copy link

@auaustorg-ms I see. Yeah it's not a bad idea and could be a good solution. It does bring in some good semantics for this specific use-case, which might prove helpful. I do agree with some others here that it adds complexity to the API for a non-typical use-case, though.

I think fixing any of the bugs that exist from nested routers, as well as remove the explicit restriction of nested Memory/History routers would be a good start. That would at least allow you and me to implement nested routers in our own ways without adding complexity to the core API.

@auaustorg-ms
Copy link
Author

@ryanflorence @timdorr I just caught this PR up to the latest changes. The functionality added in this PR is important to other users of react-router as well. I was hoping to get a better understanding of where you stand on the proposed changes in this PR? Ideally, we would love to merge these changes into react-router. At this time, we plan to maintain a fork with this additional functionality with the hopes to one day work with you to get this (or a version of this) functionality back into the project itself as we believe it will benefit a lot of users.

I look forward to hearing back. Thank you!

@oleg-codaio
Copy link

@auaustorg-ms mind linking to the npm module or repo for the fork in the meantime? It looks like the maintainers of this repo have gone silent but I'll +1 to exploring a solution for this that can be merged into react-router.

@checkrcarter
Copy link

checkrcarter commented Nov 3, 2022

Just wanted to ping on this as well. We're using a MemoryRouter with an embedded application, but want to expose different embedded versions at different browser routes. We're surprised this feature isn't supported in v6!

@pkl
Copy link

pkl commented Nov 16, 2022

Would it be easier/possible to nest a MemoryRouter (Modal) inside a BrowserRouter today if you didn't need to link back to the BrowserRouter?

@auaustorg-ms
Copy link
Author

Would it be easier/possible to nest a MemoryRouter (Modal) inside a BrowserRouter today if you didn't need to link back to the BrowserRouter?

No, this is not possible today due to how Context is currently set up within react-router.

@auaustorg-ms
Copy link
Author

I saw this post: https://remix.run/blog/open-development
So I opened a new proposal here: #9601

I am hoping we can get some traction on this change. We also decided to not maintain a fork at this current time. Anyone who needs to have this functionality can use the changes here and publish their own fork. Hoping we can see a solution like this merged into react-router one day.

Thanks all!

@brophdawg11
Copy link
Contributor

👋 We're doing a little house cleaning to start the new year. Since we have a proposal for this now, and I think if that gets accepted it'll be in favor of a separate router/RouterProvider, I'm going to close this out for now but we'll keep it linked in the proposal.

Thank you for all the hard work and exploration you put into this!

@brophdawg11 brophdawg11 closed this Jan 9, 2023
@skipjack
Copy link

skipjack commented Mar 21, 2023

@brophdawg11 can you link that proposal? Sorry just catching up here but we have a similar use case where one part of the application makes more sense in a MemoryRouter but the rest of the app is in a BrowserRouter and would love to see a more seamless approach to allowing this.

Update: Ugh, I see now I just missed above 🤦‍♂️

@brophdawg11
Copy link
Contributor

Yep! #9601

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

Successfully merging this pull request may close these issues.