netlify / next-on-netlify

Build and deploy Next.js applications with Server-Side Rendering on Netlify!
MIT License
717 stars 66 forks source link

Why are Pre-rendered HTML or SSG pages given an entry in the _redirects file? #26

Closed moop-moop closed 4 years ago

moop-moop commented 4 years ago

The Netlify platform does file shadowing, where the static file is delivered first regardless: https://docs.netlify.com/routing/redirects/rewrites-proxies/#shadowing

Example in _redirects: /health/en/kids/emily-story/article /health/en/kids/emily-story/article.html 200

I ask, because we are pre-rendering + SSG many thousands (15,000+) of pages. So the _headers files has an entry for each. Also, the console log prints a line for each during build.

FinnWoelm commented 4 years ago

Hey @moop-moop,

You are totally right about these redirect being unnecessary. I will change that this weekend :blush:

Do you have a suggestion for how to improve the build output? I do want to keep the build output, so that people know what's happening. Maybe we can limit each build step to print max 50 files. And passing the option --debug to next-on-netlify could be used to print all files, if desired?

Thanks for taking the time to post this issue!

moop-moop commented 4 years ago

Hey @moop-moop,

You are totally right about these redirect being unnecessary. I will change that this weekend 😊

If redirect rules is not present, the logging won't be so long. 😉

Do you have a suggestion for how to improve the build output? I do want to keep the build output, so that people know what's happening. Maybe we can limit each build step to print max 50 files. And passing the option --debug to next-on-netlify could be used to print all files, if desired?

Excellent idea!, but I would limit any list to maybe 5 and say "X more entries.". next build sort of does that....

Thanks for taking the time to post this issue!

No... thank you! 🙇 Awesome package and a great idea by-the-way! We will be using on some large healthcare sites. I'll be sure to post anything that arises as we implement.

moop-moop commented 4 years ago

Hey @moop-moop,

You are totally right about these redirect being unnecessary. I will change that this weekend 😊

Do you have a suggestion for how to improve the build output? I do want to keep the build output, so that people know what's happening. Maybe we can limit each build step to print max 50 files. And passing the option --debug to next-on-netlify could be used to print all files, if desired?

Ahhh, my lists are so long, I just noticed the log out of the file copying too, not just the rewrite rules ....

🔥 Setting up SSG pages
5:54:19 PM:    1. Copying pre-rendered SSG pages to out_publish/

I still think 5 is enough, if there is an option for all. Whatever makes my 36,000 line log smaller helps! 😄

Thanks for taking the time to post this issue!

FinnWoelm commented 4 years ago

Hey @moop-moop,

I removed all redirects for SSG & HTML pages that are using static routing. I also added a --max-log-lines option that defaults to 50 and can be used to limit the number of lines shown in each step of the build process (i.e., 50 SSR pages, 50 SSG pages, 50 redirects, ...). If you prefer 5 lines of output, just run next-on-netlify --max-log-lines 5.

Unfortunately, I encountered a small issue with locally previewing using netlify-cli. When you have a file (e.g., test.html) and a directory with the same name (e.g., /test), you get a 403 forbidden error during local preview. It looks like the local server used by netlify-cli serves the directory rather than the file. Because directory listings are disabled, the server responds with forbidden. Everything works correctly when deployed on Netlify.

When having a redirect for every page in the _redirects file, this error does not occur. So with the changes I made today, I would essentially be adding a bug to next-on-netlify when used for local previewing. Admittedly, it's a very small bug, but I'd rather file the issue on the netlify-cli repo and get the local preview behavior fixed, so that it matches the behavior when deployed on Netlify. Until then, I'm keeping the changes on the reduce-redirects branch.

The code on the reduce-redirects branch is fully tested and works perfectly, aside from this minor issue in local preview. If you don't mind using a branch until the issue is resolved, you can do it like so:

npm install finnWoelm/next-on-netlify#reduce-redirects --save

I will keep the branch up for at least one month after it gets merged into master. Hopefully that works for you as a temporary solution.

I'll keep you posted on the issue. Hopefully it gets resolved quickly, so that we can merge this into master!

moop-moop commented 4 years ago

@FinnWoelm I understand. I have never tested that behavior with netlify dev or a live deploy for that matter. I don't see any Netlify documentation on what the behavior should be. There is also the Asset Optimization -> Pretty URLs feature, again which I have no idea if netlify dev employs or if it would impact testing for next-on-netlify image

moop-moop commented 4 years ago

Hey @moop-moop,

I removed all redirects for SSG & HTML pages that are using static routing. I also added a --max-log-lines option that defaults to 50 and can be used to limit the number of lines shown in each step of the build process (i.e., 50 SSR pages, 50 SSG pages, 50 redirects, ...). If you prefer 5 lines of output, just run next-on-netlify --max-log-lines 5.

Unfortunately, I am not sure https://www.npmjs.com/package/log-update is compatible with the Netlify logging. Locally it worked fine, but with next-on-netlify --max-log-lines 5 on Netlify propper, I saw this:

5:26:43 PM: 🔥 Copying pre-rendered SSG pages to out_publish/ and JSON data to out_publish/_next/data/
5:26:43 PM:    /healthwise/d04376a1
5:26:43 PM:    /healthwise/d05354a1
5:26:43 PM:    /healthwise/d08284a1
5:26:43 PM:    /healthwise/d04727a1
5:26:43 PM:    /healthwise/d08578a1
5:26:43 PM:    + 1 more (run next-on-netlify with --max-log-lines XX to show more or fewer l
5:26:43 PM: ines)
5:26:43 PM:    + 2 more (run next-on-netlify with --max-log-lines XX to show more or fewer l
5:26:43 PM: ines)
5:26:43 PM:    + 3 more (run next-on-netlify with --max-log-lines XX to show more or fewer l
5:26:43 PM: ines)
5:26:43 PM:    + 4 more (run next-on-netlify with --max-log-lines XX to show more or fewer l
5:26:43 PM: ines)
5:26:43 PM:    + 5 more (run next-on-netlify with --max-log-lines XX to show more or fewer l
5:26:43 PM: ines)
5:26:43 PM:    + 6 more (run next-on-netlify with --max-log-lines XX to show more or fewer l
5:26:43 PM: ines)

Unfortunately, I encountered a small issue with locally previewing using netlify-cli. When you have a file (e.g., test.html) and a directory with the same name (e.g., /test), you get a 403 forbidden error during local preview. It looks like the local server used by netlify-cli serves the directory rather than the file. Because directory listings are disabled, the server responds with forbidden. Everything works correctly when deployed on Netlify.

When having a redirect for every page in the _redirects file, this error does not occur. So with the changes I made today, I would essentially be adding a bug to next-on-netlify when used for local previewing. Admittedly, it's a very small bug, but I'd rather file the issue on the netlify-cli repo and get the local preview behavior fixed, so that it matches the behavior when deployed on Netlify. Until then, I'm keeping the changes on the reduce-redirects branch.

The code on the reduce-redirects branch is fully tested and works perfectly, aside from this minor issue in local preview. If you don't mind using a branch until the issue is resolved, you can do it like so:

npm install finnWoelm/next-on-netlify#reduce-redirects --save

I will keep the branch up for at least one month after it gets merged into master. Hopefully that works for you as a temporary solution.

I'll keep you posted on the issue. Hopefully it gets resolved quickly, so that we can merge this into master!

FinnWoelm commented 4 years ago

Oh, you are absolutely right. I wonder if there is a way to have an updating console.log on Netlify. I will look into it and report back asap!

moop-moop commented 4 years ago

@FinnWoelm

Netlify support/engineering provided me with the following information about large numbers of redirects/rewrites:

We do recommend a max of 10k redirects, and under 1000 if you can. The reason for this is that we parse the entire redirects list looking for a match on any new (uncached) content, on every page load. ... Will that many break your site? Probably not; I've seen a customer use around that many redirects before. Will it slow your site loads down? Potentially by a few hundred milliseconds for every asset so...probably worth trying to optimize.

What do you think of another special argument/flag to not write out static file redirects?

This would allow most users to ignore, and not notice the behavior difference (bug) you noticed in netlify dev and Netlify proper. In my case, I would use with the understanding that netlify dev behavior with static files may behave differently that Netlify proper.

Thanks for listening!

moop-moop commented 4 years ago

@FinnWoelm

Netlify support/engineering provided me with the following information about large numbers of redirects/rewrites:

We do recommend a max of 10k redirects, and under 1000 if you can. The reason for this is that we parse the entire redirects list looking for a match on any new (uncached) content, on every page load. ... Will that many break your site? Probably not; I've seen a customer use around that many redirects before. Will it slow your site loads down? Potentially by a few hundred milliseconds for every asset so...probably worth trying to optimize.

What do you think of another special argument/flag to not write out static file redirects?

This would allow most users to ignore, and not notice the behavior difference (bug) you noticed in netlify dev and Netlify proper. In my case, I would use with the understanding that netlify dev behavior with static files may behave differently that Netlify proper.

Thanks for listening!

I'll have 16-17k static files in one build for reference.

FinnWoelm commented 4 years ago

Hi @moop-moop,

Unfortunately, I am not sure https://www.npmjs.com/package/log-update is compatible with the Netlify logging.

This is now fixed in the reduce-redirects branch! log-update is gone and next-on-netlify now uses a simple console.log at the end of each build step to indicate the number of hidden items, if any.

Screenshot from 2020-07-26 10-50-09

What do you think of another special argument/flag to not write out static file redirects?

I just filed an issue with Netlify about the inconsistency between netlify dev and deployed Netlify: https://github.com/netlify/cli/issues/1017

Based on previous interactions with the Netlify team, my sense is that they will be eager to resolve this difference and make netlify dev match the behavior of deployed Netlify. As soon as that is done, reduce-redirects will be merged into master and become the default behavior for anyone using next-on-netlify.

If the Netlify team does not resolve the difference, then I think your idea of a special argument is a great idea. Something like --no-static-redirects.

How does that sound to you? :blush: Finn

FinnWoelm commented 4 years ago

Hey @moop-moop,

The netlify dev issue that was blocking this branch from merging into master was just resolved: https://github.com/netlify/cli/issues/1017

Once the updated version of netlify-cli is released, I will merge this branch into master :tada:

Thanks for your patience!

moop-moop commented 4 years ago

Hey @moop-moop,

The netlify dev issue that was blocking this branch from merging into master was just resolved: netlify/cli#1017

Once the updated version of netlify-cli is released, I will merge this branch into master 🎉

Thanks for your patience!

Thank you for all your work and effort! 🥇

FinnWoelm commented 4 years ago

Good news: The fix was merged into netlify/cli! I just merged reduce-redirects into master and released it as next-on-netlify v2.4.0 today! :tada:

If you're previewing locally, make sure to use netlify/cli v2.62.0 or higher.

I will close this for now — feel free to reopen at any point. Happy hacking! :fire:

PS: I'll keep the reduce-redirects branch up until October 6th. Make sure to switch back to next-on-netlify when you get a chance :slightly_smiling_face:

moop-moop commented 3 years ago

I added next-on-netlify 2.6.3 to new project and it's creating a redirect in the _redirects for every static page again? This is a problem with tens of thousands of redirects.

FinnWoelm commented 3 years ago

Hey @moop-moop, I believe that those redirects are there to support preview mode. Can you double check? They should have some condition mentioned with a _next... cookie. The redirects are there to make Next.js preview mode possible.

It's a performance issue for you, right? Does your site need preview mode? Maybe we need a --no-preview-mode option, that does not add those redirects... Thoughts?

cc @lindsaylevine

moop-moop commented 3 years ago

That was my conclusion after looking into this too. The large site I was exploring (with many dynamic routes) had all next.js pages explicitly set to fallback:false So in order to get a "preview" for a static page, there must a rule to bypass the static files it would seem.

I was not able to test if the same happens when fallback:true due to other problems I can't solve right now.

I wonder if it's possible to create the preview mode rewrites based on the next.js pages with splats, rather that for every generated dynamic route.

We would like to explore preview mode too, but can't handle the tens of thousands of rules that will get created right now.

lindsaylevine commented 3 years ago

hey nathan! @moop-moop yeah, great callout. i actually identified this in my ongoing i18n work, which unfortunately has to add even more redirects, but this PR is aiming to use splats/placeholders like you mentioned instead of adding a redirect for every prerendered path. the code itself in that PR is in a messy place right now but starting to clean it up.

also, i still have to address how dataRoutes are each getting their own redirects (and i saw your question on the next repo). feel free to open a new issue about this (since this one is more of an older question/discussion and less of an action item) or follow along on the i18n PR progress.

lindsaylevine commented 3 years ago

@moop-moop hey update from my end. i'm getting ready to close out my i18n work and think the scope of updating non-i18n static redirects to be more performant doesnt fit into that PR. (i did write logic to limit i18n-based redirects in the way that ill apply to default apps, but i just don't want to push out too much at once). that said, i opened a new issue for it and am tagging you there. cheers! https://github.com/netlify/next-on-netlify/issues/123