Skip to content

Conversation

james-elicx
Copy link
Collaborator

@james-elicx james-elicx commented Oct 6, 2024

Note

This PR depends on #64 being merged, and then rebased with main, to be utilised properly.

Context

At the moment, static routes are being SSR'd, despite getting pre-rendered at build-time in the app directory. This means we have a series of static assets that the Next.js server wants to be able to serve during rendering. At the moment, there is no way for the incremental cache to retrieve these static assets. This is because we have a cache handler that doesn't interact with the file-system for the seed data.

Changes


Future thought: Can asset fetchers expose a way to list the assets that they have within them, or to check the existence of an asset instantly?

@james-elicx james-elicx changed the title feat: use incremental cache during rendering feat: ssg support through seeding incremental cache Oct 6, 2024
@james-elicx james-elicx changed the title feat: ssg support through seeding incremental cache feat: ssg support through seeding the incremental cache Oct 6, 2024
Copy link

pkg-pr-new bot commented Oct 6, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/@opennextjs/cloudflare@65

commit: 1ea32a8

@james-elicx james-elicx force-pushed the james/seed-incremental-cache branch 6 times, most recently from 8892ccd to f60dbc3 Compare October 6, 2024 22:17
@james-elicx james-elicx marked this pull request as ready for review October 6, 2024 22:18
@james-elicx james-elicx force-pushed the james/seed-incremental-cache branch from f60dbc3 to ba6e5f0 Compare October 6, 2024 22:21
@vicb
Copy link
Contributor

vicb commented Oct 7, 2024

Future thought: Can asset fetchers expose a way to list the assets that they have within them, or to check the existence of an asset instantly?

We can generate an index at build time but at some point we should populate the KV when building the app

Copy link
Contributor

@vicb vicb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, thanks for the PR!

I added a few comments. Please address before we merge the PR.

Copy link

changeset-bot bot commented Oct 7, 2024

⚠️ No Changeset found

Latest commit: 1ea32a8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@james-elicx
Copy link
Collaborator Author

We can generate an index at build time but at some point we should populate the KV when building the app

My question is around asset bindings exposing a way instead of us generating an index. I recall a year or so a go, Greg posed the question of would it be useful if the Pages asset binding had a way to check if assets existed / get a list. I don't think anything ever came from that though.

@james-elicx james-elicx requested a review from vicb October 7, 2024 07:33
@james-elicx james-elicx requested a review from vicb October 7, 2024 08:05
@vicb vicb merged commit 04be59b into opennextjs:main Oct 7, 2024
7 checks passed
@vicb
Copy link
Contributor

vicb commented Oct 7, 2024

Thanks!


const seedKey = `http://assets.local/${SEED_DATA_DIR}/${key}`.replace(/\/\//g, "/");

if (ctx?.kind === "APP" || ctx?.kind === "APP_ROUTE") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be ctx?.kind === "ROUTE" instead of APP ?

Copy link
Collaborator Author

@james-elicx james-elicx Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in app directory in older versions of next 14, it uses APP for requests that should be able to match .body (e.g. sitemaps)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not checked pages directory, so that might apply there, but we list pages dir as unsupported at the moment)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, i missed the parseCtx function.
The .body is only for cached app route anyway, so that's fine

return null;
}

async set(...args: Parameters<CacheHandler["set"]>) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ISR revalidation runs in the background, it doesn't look like there is anything awaiting for the revalidation to finish

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i haven't changed this part - this pr was only implementing seed data. we dont support isr yet from my testing

@james-elicx james-elicx deleted the james/seed-incremental-cache branch October 7, 2024 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants