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

RR 7 can't prefetch link with intent set to viewport #12439

Closed
raRaRa opened this issue Dec 2, 2024 · 10 comments · Fixed by #12485
Closed

RR 7 can't prefetch link with intent set to viewport #12439

raRaRa opened this issue Dec 2, 2024 · 10 comments · Fixed by #12485
Labels
awaiting release This issue have been fixed and will be released soon bug

Comments

@raRaRa
Copy link

raRaRa commented Dec 2, 2024

I'm using React Router as a...

framework

Reproduction

Open https://stackblitz.com/edit/prefetch-viewport-bug?file=app%2Froutes%2Fhome.tsx

Check console logs for this warning:
Tried to prefetch /slideshows/test/play but no routes matched.

This only happens when the <Link> has the prefetch attribute set to viewport

System Info

System:
    OS: macOS 15.1.1
    CPU: (10) arm64 Apple M1 Pro
    Memory: 127.64 MB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.9.0 - ~/.nvm/versions/node/v20.9.0/bin/node
    Yarn: 1.22.22 - /opt/homebrew/bin/yarn
    npm: 10.8.1 - ~/.nvm/versions/node/v20.9.0/bin/npm
    pnpm: 9.14.4 - /opt/homebrew/bin/pnpm
    Watchman: 2024.11.25.00 - /opt/homebrew/bin/watchman
  Browsers:
    Chrome: 131.0.6778.86
    Safari: 18.1.1
  npmPackages:
    @react-router/dev: ^7.0.0 => 7.0.1
    @react-router/fs-routes: ^7.0.1 => 7.0.1
    @react-router/node: ^7.0.1 => 7.0.1
    react-router: ^7.0.0 => 7.0.1
    vite: ^5.4.11 => 5.4.11

Used Package Manager

npm

Expected Behavior

No warning

Actual Behavior

Warning shows up that a route was not matched.

@raRaRa raRaRa added the bug label Dec 2, 2024
@Abhinav5383
Copy link

This same thing started happening to me in remix after enabling lazy route discovery, disabling that solves the problem

@raRaRa
Copy link
Author

raRaRa commented Dec 4, 2024

This same thing started happening to me in remix after enabling lazy route discovery, disabling that solves the problem

Oh interesting, how do you disable it?

@Abhinav5383
Copy link

Oh interesting, how do you disable it?

In remix there's future flags that can be switched on or off in the vite config, (according to the docs all these things are going to be default in RR v7 though)

export default defineConfig({
    plugins: [
        remix({
            future: {
                ... Other things
                v3_lazyRouteDiscovery: false,
            },
        }),
        tsconfigPaths(),
    ],
});

@raRaRa
Copy link
Author

raRaRa commented Dec 4, 2024

Ah gotcha, I was hoping I could disable it in RR 7.

@brookslybrand
Copy link
Contributor

You can't disable it in RR7, the purpose of future flags is to allow you to opt into things that will be default behavior in the next version.

This does seem weird though, assigning it to @brophdawg11 to take a look

@brophdawg11
Copy link
Contributor

This warning is correct and something we may want to suppress, but the prefetching is working correctly in this example 🙂

With the lazy discovery we don't have routes immediately, so on the first render of <Link prefetch="viewport"> if it's already in the viewport, that route is not yet known to the client, so it can't be matched and the prefetch logic bails early with that warning.

At the same time, when that Link is rendered we see the destination path and make a /__manifest request that updates the client side routes with the play.tsx route.

Upon updating of the client side route tree, the Link is re-rendered and the refetch logic runs again, correctly matches the route, and prefetches as expected:

Screenshot 2024-12-06 at 10 07 41 AM

So, the warning is accurate, albeit a bit confusing in a lazy route discovery world. We probably just want to remove the warning now that lazy discovery is the default but need to think a bit more to ensure that doesn't suppress valid instances of the warning.

Copy link
Contributor

🤖 Hello there,

We just published version 7.1.0-pre.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!

@brophdawg11 brophdawg11 added the awaiting release This issue have been fixed and will be released soon label Dec 18, 2024
@brophdawg11 brophdawg11 removed their assignment Dec 18, 2024
Copy link
Contributor

🤖 Hello there,

We just published version 7.1.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!

Copy link
Contributor

🤖 Hello there,

We just published version 6.28.2-pre.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!

Copy link
Contributor

🤖 Hello there,

We just published version 6.28.3-pre-v6.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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting release This issue have been fixed and will be released soon bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants