-
Notifications
You must be signed in to change notification settings - Fork 29
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
docs: ADR 59: Incremental static regeneration #5863
Conversation
Changes unknown |
docs/arch/adr-59.md
Outdated
When the first Engaging Crowds projects launched, project pages were built dynamically with `getServerSideProps`. Page builds were slow, with pages sometimes taking several seconds to load in the browser ([#2741](https://github.com/zooniverse/front-end-monorepo/issues/2741).) | ||
|
||
## Decision | ||
Use [Incremental Static Regeneration](https://nextjs.org/docs/pages/building-your-application/data-fetching/incremental-static-regeneration) (ISR) to build project pages. ISR is a feature of Next.js, which allows page HTML and JSON to be built and cached on demand, then served with `stale-while-revalidate` cache contol headers. While a page is building, visitors will get stale, cached content, resulting in a faster experience. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Serving static content from the CDN is also financially cheaper for Zooniverse, compared with serving HTML and JSON responses from the Next.js origin server.
40d6779
to
ecddf8d
Compare
docs/arch/adr-59.md
Outdated
## Consequences | ||
- Page builds were switched from `getServerSideProps` to `getStaticProps`, with a revalidation time of 60s. Revalidation can be set individually for each page route. | ||
- `getStaticProps` can still be slow, when it runs, so data-fetching from the Panoptes API may need further optimisation ([#3341](https://github.com/zooniverse/front-end-monorepo/issues/3341).) | ||
- Production projects and staging projects each have their own static routes: `/production/:owner/:project` and `/staging/:owner/:project`. Next.js middleware rewrites incoming request URLs to the production and staging routes used by the project app eg. https://fe-project.preview.zooniverse.org/projects/production/nora-dot-eisner/planet-hunters-tess |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In middleware.js, I see .rewrite()
when an incoming request URL has a locale param, but there's also rules using .next()
instead of redirect:
front-end-monorepo/packages/app-project/middleware.js
Lines 49 to 66 in 54bd14a
if (pathname.startsWith('/production')) { | |
return NextResponse.next() | |
} | |
if (pathname.startsWith('/staging')) { | |
return NextResponse.next() | |
} | |
/* | |
Project pages are served from /projects/staging/[owner]/[project] | |
and /projects/production/[owner]/[project] | |
*/ | |
url.pathname = `/${panoptesEnv}${pathname}` | |
if (url.locale) { | |
url.href = `${url.origin}/projects/${url.locale}/${panoptesEnv}${pathname}` | |
} | |
return NextResponse.rewrite(url) | |
} |
Would you be willing to add a short comment in middleware.js that explains the difference between.next()
and redirect()
in relation to your point here about rewriting to production and staging routes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Response.next()
carries on routing without modifying the response. Here, it’s called when a URL doesn’t need rewriting.
https://nextjs.org/docs/app/api-reference/functions/next-response#next
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an example of a link that won’t be rewritten:
https://fe-project.preview.zooniverse.org/projects/production/nora-dot-eisner/planet-hunters-tess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the middleware again, I might rearrange it, so that all the responses that should not be rewritten are at the top, followed by the responses that are rewritten.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've rewritten the item about rewrites, rearranged the middleware, and added more comments to middleware.js
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect - thank you!
f41bb45
to
6500716
Compare
|
||
## Consequences | ||
- Page builds were switched from `getServerSideProps` to `getStaticProps`, with a revalidation time of 60s. Revalidation can be set individually for each page route. | ||
- `getStaticProps` can still be slow, when it runs, so data-fetching from the Panoptes API may need further optimisation ([#3341](https://github.com/zooniverse/front-end-monorepo/issues/3341).) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this also be a cold-start issue when the function first runs? Similar to vercel/next.js#12447.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about that, as the slowness persists after the first call. Here's a screenshot of the network tab for Deciphering Secrets, showing slow requests for JSON page props (marked with a turtle icon in Firefox dev tools.)
Next.js prefetching probably doesn't help here, in that page props are fetched whenever you hover over a link. getStaticProps
will run for those links if the cache is stale.
A quick ADR to describe the consequences of static project page builds, from Engaging Crowds.
A quick ADR to describe the consequences of static project page builds, from Engaging Crowds.
Please request review from
@zooniverse/frontend
team or an individual member of that team.Linked Issue and/or Talk Post