Skip to content
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

[ECO-2243] Fix NextJS config and cache #283

Merged
merged 17 commits into from
Nov 7, 2024
Merged

[ECO-2243] Fix NextJS config and cache #283

merged 17 commits into from
Nov 7, 2024

Conversation

CRBl69
Copy link
Collaborator

@CRBl69 CRBl69 commented Oct 4, 2024

Description

This PR fixes the NextJS caching configuration and routing configuration:

  • Caching:
    • revalidate is hard coded to 2 as NextJS does not support setting revalidate to dynamic values.
    • force-static and force-dynamic are removed since they're mostly useless.
  • Routing:
    • app/page.tsx and app/home/page.tsx were duplicates. With the new configuration changes, app/page.tsx is removed (actually, replaced with a loading page just in case anyone manages to land here) and / redirects to /home.

Testing

See vercel (also vercel logs for cache).

Checklist

  • Did you check all checkboxes from the linked Linear task?

Copy link

vercel bot commented Oct 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
emojicoin-dot-fun ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 7, 2024 11:23pm
emojicoin-dot-fun-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 7, 2024 11:23pm

Copy link
Collaborator

@xbtmatt xbtmatt left a comment

Choose a reason for hiding this comment

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

I'm a little confused why we are switching from forcing all requests to be static by default caching everything. As far as I understand, caching requests will cache them indefinitely unless revalidate is set. Setting it to 1 should work for fetches on that page, but I'm not sure about requests from any other part of the route tree or layout.

Also, did you test the variable revalidate amount and find that it didn't matter what you set it as? Because I have seen the value changed in the .next/fetch/* files. And the docs here suggest it's valid to use a number: https://nextjs.org/docs/app/api-reference/file-conventions/route-segment-config#revalidate

I believe that it's only ignored in the edge runtime, but I believe that's only used in our middleware right now (I could be wrong about this).

In general though there are many cases where we really don't want to cache at all, and some where we want to cache much longer than 1 second. I think if we want to implement caching in any capacity, we should have e2e tests for it and a thorough investigation into how it works.

That is, until we have an extensive understanding of how caching works, we should probably just default to no caching at all, and it seems like this is the opposite of that.

In other words, I think we should default to const fetchCache: 'default-no-store' if we're going to do anything

Copy link
Collaborator

@xbtmatt xbtmatt left a comment

Choose a reason for hiding this comment

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

Just commenting here to reiterate that the main thing to test is all server fetches used in files with the "use server"; directive at the top, since they are all essentially overloaded nextjs fetch functions.

The main thing is to see what router path they're included in, since the file can be completely disparate from the file-based directory that normally resolves the path for caching

Copy link
Collaborator

@xbtmatt xbtmatt left a comment

Choose a reason for hiding this comment

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

This is working pretty well! Thanks for implementing this- I appreciate you getting this stuff to work with a more simple/elegant solution than using unstable_cache everywhere

@xbtmatt xbtmatt enabled auto-merge (squash) November 7, 2024 23:23
@xbtmatt xbtmatt merged commit 5a82891 into main Nov 7, 2024
8 checks passed
@xbtmatt xbtmatt deleted the ECO-2243 branch November 7, 2024 23:27
xbtmatt added a commit that referenced this pull request Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size-medium A medium sized PR. This can be reviewed and merged in a moderate amount of time. urgency-high Please check first
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants