-
-
Notifications
You must be signed in to change notification settings - Fork 457
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
Upload the prerendered JSON files into S3 #390
Upload the prerendered JSON files into S3 #390
Conversation
I tried to redeploy using this PR but CloudFront uses the old cache somehow and pages suppose to be updated are not updating. |
Thanks for looking into this @mertyildiran !
The cache behaviour in your link is for the old version of this project.
There is a PR that will allow configuring the cache behaviour TTLs etc > #282
We have to be careful about this, since serverless-next apps are replicated across the globe, we can only cleanup old builds once Cloudfront has fully propagated the changes, so it may not be as easy as it seems. I think is better to handle that on a separate issue / PR. |
One more thing, could you add a test like this one 🙏 ? |
@danielcondemarin OK, I've added a test.
☝️ OK, I think people will have to use serverless-cloudfront-invalidate package to invalidate the cache if they want to invalidate. So I see now that's not an issue of this PR.
Great! 👍
Cached pages do not come from S3, I've tested that just a moment ago by deleting all of the old build folders. If the page is cached, it will be fetched directly from CloudFront CDN, otherwise AWS will pull it from S3. That's what I understand, I might be wrong. Sure we can discuss on another issue or PR. |
@mertyildiran Is this PR ready ✅ now? |
@danielcondemarin I was afraid that upload time will take too long but for ~4000 pages but it got only 0.4% slower. I'm not sure what happens if we hit huge numbers though. Is it possible to bulk upload to S3 or upload in parallel(if it's not in parallel already)? As I said in here, ZEIT Now has some sort of mechanism to cache the pages by default. I mean even if you are the first visitor of the page, because the page is cached before-hand, you get the HTML instantly. Maybe it's related to #355 (comment) Check this website of mine which I've deployed to ZEIT Now, for example. Everything is instantaneous in that website. How can we achieve the same result with |
Only 0.4% slower is awesome! It already should be uploading to S3 in parallel. As you may have seen on top of that it tries to use S3 accelerated uploads which is much faster.
AFAIK I know CloudFront doesn't support what you are describing. Do you have any reference |
@danielcondemarin actually I tried to calculate the elapsed time with two different approaches and I get 200 - 300 seconds for uploading 9k pages, on a 5 mbps upload speed connection. With the same internet connection, deployment of 9k pages took 2265 seconds.
I don't have any proof but I rely on my observations. Maybe I should visit all pages(programmatically) after every deployment(via a shell script for example) and set |
Do you know how long it takes when deploying to Zeit NOW? Uploads to S3 are async as you can see here. Not sure how else we could optimise here tbh. Since this task is IO and not CPU bound, creating a few more node processes won't make much difference.
You could do that but bear in mind that would only result in caching at one of CloudFront's points of presence / edge location and possibly a regional cache as well. For minimum TTL I'd recommend you set it to one year if you want it to not expire. This RFC recommends doing that.
|
@danielcondemarin I cannot deploy to ZEIT Now because of maximum 24 Serverless Functions per Deployment limit. 😄 There is a package called s3-batch-upload but I'm not sure how usable it's in this scenario. OK, I'm gonna set |
@danielcondemarin by the way, I'm gonna do a few more checks before marking this PR Ready. |
@danielcondemarin I put |
Did you set |
Did you set Also, could you check if there is a |
@danielcondemarin I return return { paths, fallback: false }; There are If you're talking about the existence of |
Right, so I have a theory of what is happening: Because next.js outputs a For example: "dynamicRoutes": {
"/post/[id]": {
"routeRegex": "...",
"dataRoute": "/_next/data/123/post/[id].json",
"fallback": false,
"dataRouteRegex": "..."
} We should copy the pre-rendered HTML page and its associated JSON props file:
In summary, you'll need to change a few things:
I may be missing something here, but its the best I can do in 15 mins of writing 😄 Btw: I think I may start a disqus chat group for people to be able to join, especially contributors like yourself. That way communication happens quicker 💨 EDIT You shouldn't actually need to skip copying the .js page file to the Lambda artefact considering that on step 3. the check for the prerender manifest can happen before attempting to SSR anything ;) |
@danielcondemarin thanks for the detailed guide. I can do 1 and 2, but 3 is beyond my understanding of inner workflows of |
Yes! I’m happy to. Just let me know once the prerender-manifest is being uploaded and I’ll jump on the PR |
@danielcondemarin I did 1 and 2, although; I added this function sort of a duplicate of index.ts#L52-L66 in terms of purpose, and a duplicate of index.ts#L72-L87 in terms of coding. I kept index.ts#L52-L66 because it seemed like it has some different purposes. I'm not sure what's the point of uploading JSON files if we're gonna upload the HTML files. I observed that the functions specified in here are executed twice. I don't know if that's the intended behaviour. Anyway, I'm not sure that if I made any mistakes but I'm sure that you'll be able to detect and fix my mistakes. 😊 |
Cheers @mertyildiran . I'll take a look tomorrow! |
Hey guys, absolutely awesome work on this by the way, I'm from a startup who's rewriting their e-commerce on next/react after a quick PoC on wordpress/woo(I feel very dirty) - we're pretty much a serverless house so this is very interesting, quick note don't add that cloudfront invalidate plugin just call By the way we'll have a about 4000 items this week that will need staticPaths/props implemented so I can try and give this a whirl on our staging env if it helps? |
I can see why you'd be confused between this function and index.ts#L52-L66 as they both upload HTML Static Pages. However, the pages uploaded from the
The JSON files are for client side transitions. Click a next <Link /> that links to a page using getStaticProps and you'll see Next.js requesting the json file:
Do you mean HTML pages uploaded in 2 places? If that's it, that's intended as I explained above.
I just had to fix the path to the HTML pages to put them in the S3 bucket static-pages/ directory and add support for the prerender-manifest to the Lambda@Edge default handler. I've done some testing of my own using getStaticPaths & getStaticProps with |
@marcfielding1 Please do test it! That would be much appreciated. Bear in mind that this PR only works for |
@danielcondemarin is there a way to skip I think I opened this PR to implement your:
description in here. But now, I think we should follow a different strategy to reduce the build and/or upload time. Hence, this PR might become irrelevant. Edit: I remember now, there is |
@mertyildiran You should be able to skip the build locally by using Also, the problem you are describing as to having a large number of pages and build scalability issues is what I don’t think this PR would be irrelevant though, it’s a good step closer to supporting the more complex workflow. |
@danielcondemarin yeah I remembered I will adjust my workflow according to |
@mertyildiran If you don't have any objections I'm planning on getting this PR merged. |
@danielcondemarin sure, feel free to do anything with this PR. |
Thanks. Out of interest did you manage to test it using |
@danielcondemarin what I'm looking for was actually Incremental Static Regeneration which is not implemented yet. So |
Makes sense. Let me know if you would like to collaborate on supporting that. More than happy to work on it with you. |
… [fallback: false] (serverless-nextjs#390) Co-authored-by: Daniel <[email protected]>
This PR implements the
getStaticProps
part of #355 forgetStaticPaths
,falback: false
There was no need to define a new cache behaviour will be created in CloudFront for the path pattern for
/_next/data/*
because this cache behaviour already covers those paths with/_next/*
wildcard.The page has to be visited for caching it. While ZEIT(Vercel) Now has some sort of mechanism to always keep it on cache. I think @danielcondemarin your #355 (comment) indicates that it's possible but with a limit. Also I guess that's why ZEIT(Vercel) Now implemented a 24 Serverless Functions per Deployment limit for Pro plan recently. For a website that with never changing static pages, on certain paths, can we make
MinTTL
infinite(or at least a huge value)?The old deployments will consume quite amount of S3 storage space, so I think there should be a clean up after the deployment. The build IDs are always 21 characters long, so maybe we can look up for directories under
/_next/data/
with 21 characters name and not equal to current build ID.