vercel / next.js

The React Framework
https://nextjs.org
MIT License
126.51k stars 26.92k forks source link

Running built version ignores loading.tsx compared to dev build #47524

Open pkellner opened 1 year ago

pkellner commented 1 year ago

EDIT: I've changed the code in the repo to return a random number instead of new Date(). The dev version runs correctly in that it changes the number on every render. Running with npm run build and npm start does a premature optimization and always returns the number first generated. This is incorrect behavior.

Verify canary release

Provide environment information

Problem with Next.js 13.2 Experimental

Reproduce problem:

## Expected Behavior

Run with "npm run dev" 
Browse to localhost:3000/demo
Notice that `loading.tsx` appears for 2 seconds 
Then date is rendered as expected

## Problem Behavior

Run with "npm run build", then "npm start"
Browse to localhost:3000/demo
Notice that `loading.tsx` never appears
The date renders

GitHub Repo
https://github.com/pkellner/nextjs13-dev-vs-build-bug

This can be run in codesandbox here: https://codesandbox.io/s/github/pkellner/nextjs13-dev-vs-build-bug
(must select "use new codesandbox interface")

tested with 13.2.5-canary.16

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

No response

Link to the code that reproduces this issue

https://github.com/pkellner/nextjs13-dev-vs-build-bug

To Reproduce

Run with "npm run dev" Browse to localhost:3000/demo Notice that loading.tsx appears for 2 seconds Then random number is rendered as expected

Describe the Bug

loading.tsx not showing on "npm run build" "npm start" and also, the random number displayed does not change

Expected Behavior

expect to see loading.tsx and then a new random number

Which browser are you using? (if relevant)

chrome

How are you deploying your application? (if relevant)

tested with Mac OSX and docker

noahebrooks commented 1 year ago

Hey @pkellner, I ran your repo locally and played around with a few things and I believe this has to do with how Next.js optimizes builds for pages which are completely static. Since your delay function can be determined during build it is ran while generating the static pages (Note you should see Generating static pages (5/6) {output from delay function...} in your terminal during build. I tested my assumption and come away with a few highlights.

If you change the folder structure to make the Page a dynamic route /demo/[ms] and change the code as follows:

/demo/[ms]/page.tsx

import HeaderDemo from "./HeaderDemo"

export default function Page({ params }: any) {
  const { ms } = params;

  return (
    <>
     {
        // @ts-ignore
       <HeaderDemo ms={ms} />
     }
   </>
  )
}

/demo/[ms]/HeaderDemo.tsx

const delay = (ms: number) => {
  console.log("delay:",ms)
  return new Promise((resolve) => setTimeout(resolve, ms));
};

async function getDateAndTimeDelayed(ms: number) {
  await delay(ms)
  return new Date().toDateString();
}

export default async function HeaderDemo({ ms }: { ms: number }) {
  const now = await getDateAndTimeDelayed(ms);
  console.log(now);
  return <div>on {now}</div>;
}

After running npm build && npm start you should get the expected behavior. For example if you visit /demo/2000 you should see the loading screen for 2 seconds.

I hope this helped.

pkellner commented 1 year ago

@noahebrooks that's an interesting point and I agree it's an optmization that makes sense in my example. I've changed the example in the repo to return a random number which is a side effect that should not be optimized away (and it is).

This is clearly a premature optimization and should not be happening.

Here is the new code in the repo:

function getRandomInt(min: number, max: number) {
  min = Math.ceil(min);
  max = Math.floor(max);
  return Math.floor(Math.random() * (max - min) + min); // The maximum is exclusive and the minimum is inclusive
}

async function getRandomNumber() {
  await new Promise(resolve => setTimeout(resolve, 2000));
  const randomInt = getRandomInt(1000,3000);
  return randomInt;
}

export default async function HeaderDemo() {
  const num = await getRandomNumber();
  return <div>Random Number:{num}</div>;
}
noahebrooks commented 1 year ago

@pkellner ah yes, this is an interesting edge case. I would say anytime we deal with functions like random which are always pseudorandom implementations, which rely on some internal state, they give the illusion of being dynamic or unable to be known at build or compile time. The functions that Next.js 13 deems to be dynamic can be found here. If we would still like the desired outcome, for Next.js to treat this page as dynamic instead of static there is a really helpful export we can define in layout.tsx.

/demo/layout.tsx

export const dynamic = 'force-dynamic';

export default function DemoLayout({
  children,
}: {
  children: React.ReactNode
}) {
  return (
    <>
      {children}
    </>
  )
}

If you build and run, you should see the desired outcome. I hope this helped! Let me know if you find anything else.

knenkne commented 1 year ago

+1, I've been facing the same issue and It seems to be critical since affects FCP so much