[NEXT-1187] Link navigation with loading.tsx is not instant for dynamic pages

tonypizzicato commented 1 year ago

Verify canary release

[X] I verified that the issue exists in the latest Next.js canary release

Provide environment information

Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 22.1.0: Sun Oct  9 20:14:30 PDT 2022; root:xnu-8792.41.9~2/RELEASE_ARM64_T8103
Binaries:
  Node: 18.12.1
  npm: 8.19.2
  Yarn: 1.22.19
  pnpm: N/A
Relevant packages:
  next: 13.0.6-canary.2
  eslint-config-next: 13.0.5
  react: 18.2.0
  react-dom: 18.2.0

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

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

Link to reproduction - Issues with a link to complete (but minimal) reproduction code will be addressed faster

https://github.com/tonypizzicato/next-13-loading

To Reproduce

pull the repo
run yarn install && yarn dev
open localhost:3000 in the browser
go to the "Careers" page
reload the page
set some throttling in devtools
click different pages in the menu
notice that "Careers" page loading state and route change are done instantly but with some delay for other pages

Describe the Bug

When you have a dynamic page with loading.tsx the route change and showing the loading animation are instant only for the page, which was freshly loaded. For other dynamic pages it hits the server first, then shows the loading state

https://user-images.githubusercontent.com/640122/204661070-b9409f71-3814-45e6-bf56-923cdf4e2fc6.mov

Expected Behavior

Instantly navigate to the desired dynamic page and show loading component

Which browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

_NEXT-1187

unleashit commented 1 year ago

It may be working as designed, but it doesn't work. Relying on a lazy loaded response can and will lead to weird glitches with loaders. Separately, I would expect loading.js (and IMO suspense fallbacks) even in static/SSG sites to show the fallback if no RSC for the page or component has yet been downloaded, which can happen when prefetch prop is set to false.

It's hard to not get the feeling that Vercel has planned for a heavy use of prefetching for their own concerns and we has devs should not just brush this under the rug and give them a pass. We are more or less forced by the industry to learn and use this tool, but it can be made better if you aren't afraid to give them a little pressure about it. Prefetching all the things is a terrible idea. Worrying about adding 10-20k to a bundle is good, but never without consideration about the overall balance.

murilo1008 commented 1 year ago

I updated my project to the latest version of Next. In fact, loading has improved along with page change speed, but it is not yet in immediate development.

I used this command:

npm i next@latest react@latest react-dom@latest eslint-config-next@latest

Do I need to do anything else to make page switching instantaneous?

arackaf commented 1 year ago

@murilo1008 I'd create a simple repro and upload to Vercel. Feel free to fork mine above

leerob commented 1 year ago

I want to clarify how loading UI works in Next.js.

Routing in the App Router is "server centric". We need a network roundtrip to the server when doing route transitions. The client doesn't know what loading state to render, until it communicates with the server.

When we prefetch, we can retrieve both the route to navigate to and the loading UI. Prefetching is what makes navigations feel snappy. If you prefetch the loading UI which does not have data fetching, then you can "instantly" show this loading state, followed by the dynamic content on the page being streamed in after. This is enabled through React Suspense and streaming server-rendering as part of the App Router.

We don't believe an alternate solution where you include all of the routing variants on the client is feasible or the best solution for Next.js. For example, consider a loading state which is 100kb of HTML / possible routes. Should that be included in the initial JavaScript bundle, blocking interactivity for everything on the page until it's downloaded? Or, is it better to not block interactivity, and load it asynchronously before the next click? We still believe the current model is correct.

There's still opportunities for us to improve the experience of transitioning between loading states. For example, we can optimize further the de-duplication of data. Thanks for the reproductions here, as they'll help us dig in further and find places to optimize.

Systemcluster commented 1 year ago

Routing in the App Router is "server centric". We need a network roundtrip to the server when doing route transitions. The client doesn't know what loading state to render, until it communicates with the server.

Does this include loading states from already loaded layouts? I have a single loading.tsx with "use client" inside a layout.tsx with "use client" that is used across the whole app. Every route uses this loader. I would expect the already-loaded globally-used loader to be shown immediately without prior network roundtrip.

unleashit commented 1 year ago

Thanks for explaining the thinking. I also just came across Deep Dive: Caching and Revalidating which is also very helpful and appreciated, The docs (at least at the point I was reading them several months ago) don't go far enough into some of these subjects or explain the thinking behind them.

Personally, I wish Next.js kept to its original roots of Isomorphic code with getInitialProps. getServersideProps started this whole thing of letting Next mange client side cache which usually results in excess server requests. That I think defeats the main advantages of SPAs, which is offloading some of the expensive work to the client and the clean separation of backend concerns. I love most of the other things that got added (SSG, even the app directory and RSCs "in the right situations"), but getInitialProps was actively discouraged which IMO is mostly why the tide started turning back to the server. Not just with Next.Js, but that's when Remix, etc. started coming out. Anyhoo, it appears the same foundation is why we get loader glitches sometimes.

We don't believe an alternate solution where you include all of the routing variants on the client is feasible or the best solution for Next.js. For example, consider a loading state which is 100kb of HTML / possible routes.

I can imagine that being possible, which is why it would be ~~nice~~ amazing if you crafty people could figure out a way to make code splitting configurable

github-actions[bot] commented 12 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.

vercel / next.js