Coder Social home page Coder Social logo

Comments (18)

regisphilibert avatar regisphilibert commented on June 2, 2024 1

I feel like we’re straying away from the purpose of this ticket and I don’t want you to think I don’t appreciate the work being done here. This new adapter is amazing and in no way am I suggesting that 404 should be static at all time if you think it's a bad decision. You know best. And yes 125k/month is generous.

But your answer suggests that it’s obvious a 404 is charged on a hybrid Astro site even though all its routes but one are static. I’m talking Astro static (prerender = true) here, as this is the only control I have over what is SSR and what is Static on my project.

My point is that if 404 ends up being server side rendered on Astro hybrid projects (and charged) in spite of being set to prerender in the project, it should be advertised because no one will guess that.

It took me a while to realize what was happening in the function log.

from adapters.

Skn0tt avatar Skn0tt commented on June 2, 2024 1

No worries, I get your point :D And I agree that it's not obvious at all, especially since the previous version of the Astro adapter behaved differently on this.

in no way am I suggesting that 404 should be static at all time if you think it's a bad decision.

Well, ideally we'd honor the prerender setting on this, as you mention! Most 404 pages will be fine with static rendering, maybe some will want to do some dynamic stuff on the server to e.g. make recommendations what page the user might have wanted to go to. Point is, both should be possible, and we should definitely fix that.

In addition to that, maybe we could add a callout to the docs on this? I'll open a PR for that.

from adapters.

Skn0tt avatar Skn0tt commented on June 2, 2024 1

Hope you're enjoying your vacation! I don't perceive this issue as being super pressing, so please don't feel like you need to monitor this during your vacation. Writing down my reply for you to look at once you're back:

You said that all routes are served by the ssr function, regardless if they are assets, pre-rendered or any other statics?

No, we're setting preferStatic: true, which configures the CDN give precedence to static assets before calling the SSR function. If there's no static asset at the requested path because it will result in a 404, this means it will still go to the SSR handler and it needs to deal with it.

IIRC we decided agains app.render() handling this. I don't know for sure, but I'm pretty sure it should be the Adapter's task to handle 404.

Got it. I don't think this is documented anywhere, and app.render() always returns a valid Response object - so I assumed it handles the request just fine. Reading through the source code for app.render, it looks like I can check on the result of app.match. If it's null, we know that it's a 404 and we can serve the static page.

For the question of dynamic or static 404 it should be up to the user. If the user uses export const prerender = true;, it should be static. IIRC a 404.html should be generated in the "output" folder. In theory that should be "routable" by Netlify without hitting the ssr function, at least it is with Cloudflare.

I'm assuming that a dynamic 404 page is correctly handled by app.render, and the adapter only needs to handle the static case. Netlify Functions, once called, can't pass through the request to the CDN again like on Cloudflare - but I think we can make this work by embedding the static 404 page into the function. (Edge Functions would be able to do this, but we don't use them in the Adapter for other reasons as you're aware)

from adapters.

Skn0tt avatar Skn0tt commented on June 2, 2024

Hey Regis! Thanks for providing the repro. For any page where there's no static asset generated at buildtime, the Netlify adapter is configured to fall back to SSR. SSR seems to render the blank 404 response then. I'd assume that it incorporates pages/404.astro when doing that. It doesn't seem like your repro contains a 404.astro page, what happens if you add it?

from adapters.

regisphilibert avatar regisphilibert commented on June 2, 2024

Hi @Skn0tt! I just added a 404 but like on other projects I've seen the problem on, it has not effect. Still getting a blank response unfortunately.

from adapters.

alexanderniebuhr avatar alexanderniebuhr commented on June 2, 2024

Not sure if that is relevant here, but for Cloudflare there is SPA mode, for which a pre-rendered 404 is needed. SSR 404 pages do not work for Cloudflare and will also return a blank page. Maybe this is related.

from adapters.

regisphilibert avatar regisphilibert commented on June 2, 2024

For any page where there's no static asset generated at buildtime, the Netlify adapter is configured to fall back to SSR

I just realized some of our site would have the SSR functions invoked when someone visited a static route on that project. At first I couldn't understand what was going on since we only have one ssr route and it's only used by CMS editors.

But it turns out, we had a meta in the pointing to local missing files. So the browser would request a 404 on every visit and an ssr function would be invoked. So as you said, whenever someone (or the browser/robots etc...) will hit a 404, the ssr function will be triggered.

It's a bit of a problem considering functions invocations are limited on plans and even though that particular 404 we controlled, it's rarely in our hands who/what will mistype a URL on one of our site. So I don't think it should be taken into take into account in our monthly limits...

Any way we could simply let the default Netlify 404 behaviour take on here on static routes?

from adapters.

Skn0tt avatar Skn0tt commented on June 2, 2024

Any way we could simply let the default Netlify 404 behaviour take on here on static routes?

For Astro sites with output: "static", this is definitely the default behaviour.

For sites that include server-rendered pages (output: "server/hybrid"), we currently use the Astro-generated server code to see if there's a server-rendered page on a route that didn't match a static page before it. This massively simplifies the routing logic, and essentially reuses Astro's routing logic instead of reimplementing it with Netlify mechanisms. The big upside to this is simplicity and correctness, the big downside is that 404 pages are server-rendered.

In my experience, Netlifys limits are generous enough that an additional 404 isn't a big deal, but it always depends on the usecase I assume! What kind of site are you working on, and how tight are the limits for you on that?

I just added a 404 but like on other projects I've seen the problem on, it has not effect. Still getting a blank response unfortunately.

Interesting! I'll try to reproduce this locally, and see what's going on here that we see a blank response.

from adapters.

Skn0tt avatar Skn0tt commented on June 2, 2024

I just added a 404 but like on other projects I've seen the problem on, it has not effect. Still getting a blank response unfortunately.

I'm seeing that pages with prerender = false are properly rendered by the SSR Function, but pages with prerender = true get a blank response as you mentioned. I wonder what's the best course of action to fix this - @alexanderniebuhr curious for your thoughts as well. From my perspective, we could either:

  1. make app.render() return a redirect to /404, similar to what's shown in the SSR routing code example in the Astro docs
  2. make app.render() render the 404.astro page on demand and return that as the response body, even though it has prerender = true defined in it.
  3. embed the prerendered 404.html into the SSR function and return that

What do you think?

from adapters.

regisphilibert avatar regisphilibert commented on June 2, 2024

In my experience, Netlifys limits are generous enough that an additional 404 isn't a big deal, but it always depends on the usecase I assume! What kind of site are you working on, and how tight are the limits for you on that?

It is generous, and I suppose we could live with it, but in the context of marketing wouldn't it be "charging for 404s"?

from adapters.

Skn0tt avatar Skn0tt commented on June 2, 2024

It is generous, and I suppose we could live with it, but in the context of marketing wouldn't it be "charging for 404s"?

Yes, it's charging for 404s, because it's traffic like all other traffic. A 404 response comes from a visitor of your site. On the starter tier, 125k function executions/month are included. That's 3 executions per second, wich is more than enough for a personal site.

from adapters.

Skn0tt avatar Skn0tt commented on June 2, 2024

There we go: withastro/docs#6546

from adapters.

alexanderniebuhr avatar alexanderniebuhr commented on June 2, 2024

@Skn0tt Sorry for the late response. I'm on vacation, but trying to catch up. Some question to clarify and guide you in the right direction:

  • You said that all routes are served by the ssr function, regardless if they are assets, pre-rendered or any other statics?
    -> We do solve this with the Cloudflare adapter, by using a _routes.json to not run the ssr function for static stuff. I suggest the same for Netlify, if possible.
  • IIRC we decided agains app.render() handling this. I don't know for sure, but I'm pretty sure it should be the Adapter's task to handle 404. Worth double checking in Discord's #dev channel
  • For the question of dynamic or static 404 it should be up to the user. If the user uses export const prerender = true;, it should be static. IIRC a 404.html should be generated in the "output" folder. In theory that should be "routable" by Netlify without hitting the ssr function, at least it is with Cloudflare.

Hope this helps, I'm still monitoring this, but I might be slower to answer

from adapters.

lilnasy avatar lilnasy commented on June 2, 2024

Is there something preventing the 404.html catch-all being added? It would match the behavior with vercel as well.

from adapters.

Skn0tt avatar Skn0tt commented on June 2, 2024

Well, the big blocker for that fallback is the precedence behaviour I outlined. Essentially, you can only ever have one fallback route configured, and in the current design that's the SSR handler. In the Vercel adapter, the SSR handler isn't treated as fallback, but is only assigned to the routes where it will be running on. That makes the adapter a little more complicated, but allows the 404 site to be the fallback.

I'll provide a PR this week that ensures the customer-written 404 pages is shown properly. Then we can continue to think about if it's a problem that that page is served by a Function in some cases.

from adapters.

Skn0tt avatar Skn0tt commented on June 2, 2024

Well, "this week" came early. Here's the PR: #143

from adapters.

alexanderniebuhr avatar alexanderniebuhr commented on June 2, 2024

@Skn0tt Is this issue fixed, and can this Issue be closed?

from adapters.

Skn0tt avatar Skn0tt commented on June 2, 2024

It's fixed, yes!

from adapters.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.