Skip to content

RR7 lazyRouteDiscover (Fog of War) opt-out #12656

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

Closed
mikkpokk opened this issue Dec 29, 2024 · 16 comments · Fixed by #13451
Closed

RR7 lazyRouteDiscover (Fog of War) opt-out #12656

mikkpokk opened this issue Dec 29, 2024 · 16 comments · Fixed by #13451

Comments

@mikkpokk
Copy link

mikkpokk commented Dec 29, 2024

Missing information how to disable lazy route discover (also known as Fog of War) feature.

In my case, it causes unnecessary overhead on application which renders thousands of links (filters and products) but there are only 7 RR routes in total. Would love to disable the feature.

@mikkpokk mikkpokk added the docs label Dec 29, 2024
@mikkpokk mikkpokk changed the title [Docs]: RR7 lazyRouteDiscover [Docs]: RR7 lazyRouteDiscover (Fog of War) opt-out Dec 29, 2024
@s-abstract
Copy link

Same here. Would love to see a way to opt out the feature or configure it.

@david-szabo97
Copy link

We need more control over Fog of War. I noticed the same issue, we have a search results page where each result is a different link. React Router fetches a manifest for all the links, but it's just a single route with parameters. It would be great to have some way to exclude links from discovery and just use whatever is already loaded.

The initial manifest only contains the matched routes. Therefore a manifest is always fetched on page load. We should have an (optional) option to eagerly load those as well.

@mikkpokk
Copy link
Author

mikkpokk commented Jan 7, 2025

I digged a bit deeper in source code and it seems there is only 1 solution - use discover={'none'} property value on NavLink / Link component on most of the links and let FOW to do it's thing on single route link only.

For an example

// Main navigation
<Link to={'/'}>Home</Link>
<Link to={'/products'}>Products</Link>
<Link to={'/login'}>Login</Link>
<Link to={'/register'}>Register</Link>
...
// Filters
<Link to={'/products?color[]=blue'} discover={'none'}>Blue</Link>
<Link to={'/products?color[]=red'} discover={'none'}>Red</Link>
<Link to={'/products?color[]=red&color[]=blue'} discover={'none'}>Blue</Link> // in case red is active already
<Link to={'/products?color[]=blue&color[]=red'} discover={'none'}>Red</Link> // in case blue is active already

// Pagination
<Link to={'/products?page=2'} discover={'none'}>2</Link>

// Products
{items.map((item, key) => (
    <Link to={`/products/${item.slug}`} discover={key === 0 ? 'render' : 'none'}>
        <ProductCard item={item} />
    </Link>
))}

However, it gets really messy and hard to maintain such a mayhem project in the future. Let's hope to hear authors thoughts soon.

@miguelvictor
Copy link
Contributor

I agree, adding discover="none" to all <Links /> is so hard. There should be a global setting to disable lazyRouteDiscovery.

@skyqrose
Copy link

+1. In my case it's causing my tests to fail during my v7 upgrade, because my assertions didn't expect the new data-discover="true" on every link.

Example of a typical test failure in a jest snapshot test
FAIL  tests/components/myComponent.test.tsx
  ● renders

    expect(received).toMatchSnapshot()

    Snapshot name: `renders 1`

    - Snapshot  - 0
    + Received  + 1

    @@ -43,10 +43,11 @@
        <a
          class="classes"
    +     data-discover="true"
          href="/path"
        >
          Link Text
        </a>
      </li>

      23 |   const view = render(node, options);
    > 24 |   expect(view.container.firstChild).toMatchSnapshot();
         |                                     ^
      25 | };
      26 |

      at expectMatchesSnapshot (tests/testHelpers/snapshot.ts:24:37)
      at Object.<anonymous> (tests/testHelpers/snapshot.ts:14:26)

As a workaround, I have a helper file with a wrapper for the React Router Link component that sets discover="none". I had to change all my imports to use my custom Link instead, but that was easier than adding a prop to every Link.

// assets/src/util/react-router.tsx
import {
  LinkProps,
  NavLinkProps,
  Link as ReactRouterLink,
  NavLink as ReactRouterNavLink,
} from "react-router";

/**
 * Use this instead of the normal react-router Link
 *
 * React Router's lazy loading / link discovery feature
 * by default adds a `data-discover="true"` attribute to every link.
 * This opts out of that feature, but must be done for every link
 * individually, hence this helper.
 *
 * It can be removed if they ever implement a global opt-out of this feature.
 * https://github.com/remix-run/react-router/issues/12656
 */
export const Link = (props: LinkProps) => (
  <ReactRouterLink discover="none" {...props} />
);
export const NavLink = (props: NavLinkProps) => (
  <ReactRouterNavLink discover="none" {...props} />
);

If this global opt-out is added, I can use that instead and delete my wrapper.

@llezhava
Copy link

+1 for opting out of this feature. I have offline-first web-app and this feature is complicating things for me. I have to build custom manifest file based on the routes.

@mikkpokk
Copy link
Author

As a workaround, I have a helper file with a wrapper for the React Router Link component that sets discover="none". I had to change all my imports to use my custom Link instead, but that was easier than adding a prop to every Link.

You probably should not apply discover="none" to every Link or NavLink element. Doing so causes your browser to miss the entire router manifest.

@skyqrose
Copy link

My app is not large, I don't do (and don't want to do) any code splitting or lazy loading.

@mikkpokk
Copy link
Author

My app is not large, I don't do (and don't want to do) any code splitting or lazy loading.

It's nothing to do with lazy loading or code splitting. When you disable the route manifest entirely (which is what you currently do by using discover="none" on every link), you actually increase load time. When a user now clicks on any link, RR7 will first fetch the link manifest before it can do the actual job.

That's the problem with the issue: we are not able to load the whole route manifest on the initial load like before.

@phlipsterit
Copy link

This is also giving us a lot of trouble. I've even encountered a race condition which renders an error. Looks like it happens because there is a request for manifest of route A going in parallel to a navigation to route B.
We have a lot of links on our page, and it seems React-Router disables eager route discovery because of this. It does discovery when clicking a link, causing extra delay.
It would be nice to opt out of this, or at least be able to turn it off for a subtree of the routes, such that it does not try to get manifest for all links that navigate to the same route but with different params, like project/A, project/B, project/C etc.

We also have very few routes (but a lot of links with different params), so the strategy in the old Remix where it would get manifest of all routes in one request at startup would be much better for performance (and number of request to the server).

@mikkpokk
Copy link
Author

@mjackson @brophdawg11 @ryanflorence Thoughts? By today, it's clear that the issue should be categorized as a 'bug' instead of 'docs'.

@c0per
Copy link

c0per commented Apr 3, 2025

I only have a 29kb uncompressed manifest file after build. But I have a page with a lot of navigate('/some_route/:id') buttons.
It really is a serious overhead having to fetch /__manifest every time a button is clicked. Due to different id, cache doesn't really solve the problem.

Even the react router doc site is calling /__manifest every time you click to navigate. FOW sure is a huge optimization for bigger site, but for a small or medium site, the network fetch overhead is also huge.

It would to nice to opt out and load the entire manifest during initial page load.

@brophdawg11 brophdawg11 added the review flag for team review label Apr 3, 2025
@ngbrown
Copy link
Contributor

ngbrown commented Apr 8, 2025

I too would like the option to just download the entire manifest in one go instead of needing to add discover="none" to a large list of Links all going to the same module. Also, adding discover="none" incurs a waterfall penalty of fetching the manifest before the data (and module if new) on every navigation. This happens on every new id to the same module.

@wovalle
Copy link

wovalle commented Apr 23, 2025

I also want a way to disable Fog of war. I'm migrating a remix v2 app to react-router. This app has ~30 routes where in many of them we're using useFetcher to augment data. I'm encountering a lot of Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'result') which I assume is useFetcher not being able to find the path (due to the lazy discovery). If I replace useFetcher by a simple fetch everything works correcly again.

This issue provides a workaround but it doesn't work for me (the request is not properly formatted). I can debug and figure out what's wrong, but I don't want to do this workaround foreach of the routes that might be called by a useFetcher.

I don't have that many paths to begin with and the __manifest file could be easily be cached

@brophdawg11 brophdawg11 removed docs review flag for team review labels Apr 23, 2025
@brophdawg11 brophdawg11 changed the title [Docs]: RR7 lazyRouteDiscover (Fog of War) opt-out RR7 lazyRouteDiscover (Fog of War) opt-out Apr 23, 2025
@brophdawg11 brophdawg11 linked a pull request Apr 23, 2025 that will close this issue
@brophdawg11 brophdawg11 self-assigned this Apr 28, 2025
@brophdawg11
Copy link
Contributor

We introduced a new routeDiscovery config in #13451 that gives you the ability to or control the /__manifest path for lazy route discovery. The new config will be available in the next release.

@brophdawg11 brophdawg11 added the awaiting release This issue has been fixed and will be released soon label Apr 28, 2025
@brophdawg11 brophdawg11 removed their assignment Apr 28, 2025
Copy link
Contributor

github-actions bot commented May 8, 2025

🤖 Hello there,

We just published version 7.6.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions github-actions bot removed the awaiting release This issue has been fixed and will be released soon label May 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.