aws-amplify / amplify-hosting

AWS Amplify Hosting provides a Git-based workflow for deploying and hosting fullstack serverless web applications.
https://aws.amazon.com/amplify/hosting/
Apache License 2.0
460 stars 116 forks source link

[NextJS] ISR not triggered for paths returned back from getStaticPaths (prerendered pages) #2268

Closed andrewgadziksonos closed 3 years ago

andrewgadziksonos commented 3 years ago

Before opening, please confirm:

App Id

dh2ndod2t9wbt

Region

us-east-1

Amplify Console feature

Not applicable

Describe the bug

We have a few dynamic routes for various pages that utilize getStaticPaths to determine which pages we should prerender at build time. Each page template is configured with a 60 second revalidation time. We are noticing that if any prerendered page is requested after the 60 seconds has expired then there is no revalidation message sent to the SQS queue

Expected behavior

Any request that contains an uri that is configured with revalidate: N should trigger revalidation

Reproduction steps

  1. create a dynamic routed page, something pages/dynamic/[id].jsx
  2. implement getStaticPaths, and return a subset of all paths, set fallback: blocking
  3. implement getStaticProps and return revalidate: 60
  4. build and deploy app
  5. request prerendered page
  6. wait over 60 seconds, hit refresh in the browser
  7. verify that no messages were sent to SQS
  8. verify that no log messages were generated by the ISR lambda

Build Settings

version: 1
frontend:
  phases:
    preBuild:
      commands:
        - yarn install --frozen-lockfile
    build:
      commands:
        - yarn build
  artifacts:
    baseDirectory: .next
    files:
      - '**/*'
  cache:
    paths:
      - node_modules/**/*
      - .next/cache/**/*

Additional information

From what I can tell, this function looks like it starts everything

/**
 * Function called within an origin response as part of the Incremental Static
 * Regeneration logic. Returns required headers for the response, or false if
 * this response is not compatible with ISR.
 */
const getStaticRegenerationResponse = (options) => {
    ...
};

If ISR logic is truly invoked in a Origin Response function, then these functions are not triggered based on this info image

My hunch is that we're hitting one of these conditions. Maybe because of the way if-none-match and etag headers work? (I am receiving a 304 response from CloudFront, but a x-cache: Miss from cloudfront header)

swaminator commented 3 years ago

@andrewgadziksonos can you goto your build settings page in the console and see what is set for the live updates field? Wondering whether it's set to latest https://docs.aws.amazon.com/amplify/latest/userguide/server-side-rendering-amplify.html#update-app-nextjs-version

jonathanlemonsonos commented 3 years ago

@swaminator can confirm, Next.js version is set to latest in the console

Athena96 commented 3 years ago

Hi, @andrewgadziksonos @jonathanlemonsonos what version of next are you using in your package.json ? Can you try with the setting it to the newest version 11.1.2 in your package.json

andrewgadziksonos commented 3 years ago

@Athena96 We're using version 11.1.2

image

ovdahlberg commented 3 years ago

We are facing the same issue as well.

ferdingler commented 3 years ago

@andrewgadziksonos I have been trying to reproduce the issue but haven't had success. I am also using ISR with dynamic pages and catch-all pages. Here are some snippets of my app:

// pages/[...slug].tsx

export default function CatchAll({ updatedAt }) {
  return (
    <div>
        Page generated at {updatedAt}
    </div>
  );
}

export async function getStaticPaths() {
  const popularPages = [
    // The pages /foo/bar and /en-US/foo/bar will be pre-generated at build time
    { params: { slug: ["foo", "bar"] } },
    { params: { slug: ["en-US", "foo", "bar"] } },
  ];
  return {
    paths: popularPages,
    fallback: true,
  };
}

export async function getStaticProps() {
  const updatedAt = new Date().toISOString();
  return {
    revalidate: 60,
    props: {
      updatedAt,
    },
  };
}

I also have a product details page that pre-generates the top 3 popular items at build time:

// pages/product/[pid].tsx

export async function getStaticPaths() {
  const top3Items = [
    { params: { pid: "11" } },
    { params: { pid: "13" } },
    { params: { pid: "19" } },
  ];
  return {
    paths: top3Items,
    fallback: true,
  };
}

export async function getStaticProps(context) {
  const productId = context.params.pid;
  const product = await getProductById(productId);
  return {
    revalidate: 10,
    props: {
      product,
    },
  };
}

ISR seems to be working fine for me, pages are regenerated properly after the revalidation time. I am using next 11.1.2. What am I missing to accurately reproduce your scenario?

ferdingler commented 3 years ago

We are facing the same issue as well.

@ovdahlberg what is your setup? Can you provide more information?

ovdahlberg commented 3 years ago

We are facing the same issue as well.

@ovdahlberg what is your setup? Can you provide more information?

Of course!

We are using version 11.0.0.

Here is an example from our code:

// pages/[slug].tsx

const PageComponent: React.FC<Props> = ({ page }) => {
  const router = useRouter();

  if (!router.isFallback && !page?.slug) {
    return <ErrorPage statusCode={404} />;
  }

  return (
    <>
      <div key={page.slug}>
        {page?.pageBuilder?.contentBlocks?.map((block) => (
          <RenderContentBlocks block={block} key={block?.uid} />
        ))}
      </div>
    </>
  );
};

export const getStaticProps: GetStaticProps = async ({ params, preview = false, locale }) => {
  const { slug } = params as { slug: string };
  const language = (locale)?.toUpperCase() ?? 'EN';

  let [page] = await getPage(slug, language);

  if (!preview && !page?.slug) {
    return {
      notFound: true,
      revalidate: 60,
    };
  }

  return {
    props: {
      page,
    },
    revalidate: 60,
  };
};

export const getStaticPaths: GetStaticPaths = async () => {
  const pages = await getAllPages();

  const paths = pages.map((page) => ({
    params: {
      slug: page.slug,
    },
    locale: page.language.slug,
  }));

  return {
    paths,
    fallback: 'blocking',
  };
};

export default PageComponent;
andrewgadziksonos commented 3 years ago

@ferdingler

I've invited you to https://github.com/andrewgadziksonos/aws-amplify-isr-issue where I was able to reproduce the issue on Amplify

The Amplify App ID is d3hn9uqs8h21cb

I took your example, and added i18n config to the next.config.js file. The prerendered route, /foo/bar will not revalidate when hitting this URL path (hitting the default locale)

https://master.d3hn9uqs8h21cb.amplifyapp.com/foo/bar https://master.d3hn9uqs8h21cb.amplifyapp.com/en-us/foo/bar

This non-prerendered route does revalidate,

https://master.d3hn9uqs8h21cb.amplifyapp.com/fr-fr/foo/bar

I hope this helps 🤞

ferdingler commented 3 years ago

@andrewgadziksonos thank you for creating the example repository.

I deployed it to my account and both routes /foo/bar and /en-US/foo/bar are regenerating properly after the revalidation time: https://master.dh9w7ke3612hv.amplifyapp.com/foo/bar https://master.dh9w7ke3612hv.amplifyapp.com/en-US/foo/bar

One thing I noted in your app d3hn9uqs8h21cb is that you have set the NextJS override version to 10 instead of latest. Can you make sure you change this value to latest and re-deploy? You can find this value under Build Settings as the following screenshot shows:

Screen Shot 2021-10-04 at 1 53 27 PM
ferdingler commented 3 years ago

One thing I noted in your app d3hn9uqs8h21cb is that you have set the NextJS override version to 10 instead of latest. Can you make sure you change this value to latest and re-deploy? You can find this value under Build Settings as the following screenshot shows:

@ovdahlberg can you check as well if you have set this to latest please?

We will improve our code to automatically detect the version of NextJS you are using, so that you don't have to do this manually. Apologies for the inconvenience.

andrewgadziksonos commented 3 years ago

@andrewgadziksonos thank you for creating the example repository.

I deployed it to my account and both routes /foo/bar and /en-US/foo/bar are regenerating properly after the revalidation time:

https://master.dh9w7ke3612hv.amplifyapp.com/foo/bar

https://master.dh9w7ke3612hv.amplifyapp.com/en-US/foo/bar

One thing I noted in your app d3hn9uqs8h21cb is that you have set the NextJS override version to 10 instead of latest. Can you make sure you change this value to latest and re-deploy? You can find this value under Build Settings as the following screenshot shows:

Screen Shot 2021-10-04 at 1 53 27 PM

@ferdingler That's really odd, it's set as latest for me in the console already. I made sure of this when I created the app

image

ferdingler commented 3 years ago

@andrewgadziksonos, that's interesting indeed! From my end, I can see that you have the LIVE_UPDATES environment variable with both values 10 and latest:

[
  {"pkg":"next-version","type":"internal","version":"latest"},
  "{\"pkg\":\"next-version\",\"type\":\"internal\",\"version\":\"10\"}"
]

Can you check the Environment Variables settings on your app and make sure you only have 1 value configured as the following screenshot shows.

Screen Shot 2021-10-05 at 5 14 31 PM

When you update that, please trigger a new deployment. It should work. It definitely works for me. The NextJS version override is important because we deploy your application based on that value, so my guess is that your current settings are causing an edge case where the latest value is being ignored.

jonathanlemonsonos commented 3 years ago

@ferdingler @andrewgadziksonos

In our main application we have the following set:

Screen Shot 2021-10-06 at 9 17 08 AM

ovdahlberg commented 3 years ago

Hi @ferdingler!

From our side it works perfectly when we changed from version 10 to latest manually.

So it seems like Amplify did't read the version of Next.js correctly.

Thanks for the help!

andrewgadziksonos commented 3 years ago

@ferdingler

Good find! I fixed the LIVE_UPDATES env var, and redeployed - my example app is now revalidating as expected...back to the drawing board. Keep you posted

andrewgadziksonos commented 3 years ago

The last thing I could think of was rewrites, but thats not the case (I'm still able to revalidate). Starting to wonder if our amplify app was in a weird state for a few days based on the timeline of this comment https://github.com/aws-amplify/amplify-console/issues/2175#issuecomment-924984355 (I just checked and all of our lambda's are now configured with a 30 sec timeout)

I'm going to re-add prerendered routes back into our main application to see if it works again 🤞

Will report back here

incraigulous commented 2 years ago

I'm having this issue as well. Revalidation works for nonstatic paths, but not for paths defined with getStaticPaths. I do have my next version set to latest. Is anyone except me still having this issue?

hectorakemp commented 2 years ago

I'm having this issue too, @incraigulous. Here's my LIVE_UPDATES for reference. I'm on Next 11.1.2.

[{"name":"Amplify   CLI","pkg":"@aws-amplify/cli","type":"npm","version":"latest"},"{\"pkg\":\"next-version\",\"type\":\"internal\",\"version\":\"latest\"}",{"name":"Next.js  version","pkg":"next-version","type":"internal","version":"latest"}]
hectorakemp commented 2 years ago

I also can confirm that setting LIVE_UPDATES to this makes ISR work on getStaticPaths pages:

 [{"pkg":"next-version","type":"internal","version":"latest"}]
github-actions[bot] commented 2 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.