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

Update middleware.js #2624

Merged
merged 29 commits into from
Dec 16, 2024
Merged

Update middleware.js #2624

merged 29 commits into from
Dec 16, 2024

Conversation

rishi-raj-jain
Copy link
Contributor

No description provided.

Copy link

vercel bot commented Dec 11, 2024

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

Name Status Preview Comments Updated (UTC)
neon-next 🛑 Canceled (Inspect) Dec 16, 2024 10:30am

@rishi-raj-jain
Copy link
Contributor Author

@saimonkat can you help me in this PR? I want to make the homepage as static asset and not as an ISR page. Is there something we're fetching on the server that we need to revalidate on the homepage after it is deployed?

image

@saimonkat
Copy link
Collaborator

@rishi-raj-jain I guess the only one thing we need to revalidate on the homepage it's GitHub stars count. That's why we has to disable it in this PR: #2589 for error pages

But let me check, I'll research deeper soon

@rishi-raj-jain
Copy link
Contributor Author

rishi-raj-jain commented Dec 11, 2024

@rishi-raj-jain I guess the only one thing we need to revalidate on the homepage it's GitHub stars count. That's why we has to disable it in this PR: #2589 for error pages

But let me check, I'll research deeper soon

I think we can avoid that atleast for / and /home since it'd fetch it during the initial render on the server anyways.

@saimonkat
Copy link
Collaborator

@rishi-raj-jain ok as I see, you already removed revalidate from the main Homepage in this PR #2621

In this case we're free to remove it from other copies of Homepage, I updated in the last commit

@rishi-raj-jain
Copy link
Contributor Author

@saimonkat somehow it still lands up being in ISR:
Screenshot 2024-12-12 at 12 54 42 AM

@saimonkat
Copy link
Collaborator

@rishi-raj-jain are you testing the current PR Preview? Can you let me know how to test it please

@rishi-raj-jain
Copy link
Contributor Author

@rishi-raj-jain are you testing the current PR Preview? Can you let me know how to test it please

Did you get a chance at my comment at #2624 (comment)?

@rishi-raj-jain
Copy link
Contributor Author

rishi-raj-jain commented Dec 12, 2024

I think this is rooting from wordpress pages, testing.

@rishi-raj-jain
Copy link
Contributor Author

In the next output it shows that it is a static page, but somehow the homepage it still marked as ISR function when deployed on Vercel.

@saimonkat
Copy link
Collaborator

@rishi-raj-jain it looks like the Vercel issue, please check
vercel/next.js#49451

Vercel generates ISR functions for all pages in the app dir by default and occasionally tries to revalidate them at runtime

Anyway I think we good to merge this PR as soon as we don't need revalidate for home pages

@rishi-raj-jain
Copy link
Contributor Author

Hmm, I see. I have commented on the tweet. Would removing the revalidate do anything? It still will fall into ISR, right?

@rishi-raj-jain
Copy link
Contributor Author

Trying specifying no revalidate explicity via https://nextjs.org/docs/app/api-reference/file-conventions/route-segment-config#revalidate.

@rishi-raj-jain
Copy link
Contributor Author

rishi-raj-jain commented Dec 12, 2024

image
vs
Screenshot 2024-12-13 at 1 40 14 AM

shared cache is now finally being used for longer times.

@rishi-raj-jain
Copy link
Contributor Author

Screenshot 2024-12-13 at 1 59 40 AM

@saimonkat
Copy link
Collaborator

@rishi-raj-jain just noticed that we missed /home route for logged in users with neon_login_indicator to show our homepage somehow. Should we restore it?

@americano98
Copy link
Collaborator

Hi @rishi-raj-jain , could you please clarify how a client-side redirect could be faster than using middleware? For instance, if we add the neon_login_indicator cookie in your preview and reload the homepage, we first see the homepage render briefly before being redirected to the console. However, in the current middleware implementation in the main branch, there’s no initial page render; we’re redirected directly to the console if the cookie is present. Could you elaborate on the reasoning behind this approach?

@rishi-raj-jain
Copy link
Contributor Author

@rishi-raj-jain just noticed that we missed /home route for logged in users with neon_login_indicator to show our homepage somehow. Should we restore it?

I added a redirect from '/home' to '/' which takes care of it, does that work?

@rishi-raj-jain
Copy link
Contributor Author

Hi @rishi-raj-jain , could you please clarify how a client-side redirect could be faster than using middleware

It's faster for TTFB - now the state of the homepage is exactly as like a static asset, served asap.

we first see the homepage render briefly before being redirected to the console.

Exactly what the original suggestion was - PPR but here we're defaulting to immediate script on the client instead of on the server.

@saimonkat
Copy link
Collaborator

@rishi-raj-jain

I added a redirect from '/home' to '/' which takes care of it, does that work?

Now it works, thanks! I see you rolled back the middlewares file with the correct conditions
As you understood, I meant the situation when when the user logged in and went to https://neon.tech/home to see the homepage

@rishi-raj-jain rishi-raj-jain merged commit 38b6aff into main Dec 16, 2024
4 of 5 checks passed
@rishi-raj-jain rishi-raj-jain deleted the rishi-raj-jain-patch-1 branch December 16, 2024 10:29
@vercel vercel bot temporarily deployed to Preview – neon-next December 16, 2024 10:30 Inactive
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