withastro / docs

Astro documentation
https://docs.astro.build/
MIT License
1.34k stars 1.5k forks source link

Confusing hybrid mode on Cookie #7215

Closed jamesli2021 closed 4 months ago

jamesli2021 commented 8 months ago

📝 Issue Description

Unclear about hybrid mode.

📋 On which page(s) it occurs

https://docs.astro.build/en/guides/server-side-rendering/#on-demand-rendering-features

🤔 Expected Behavior

What do the readers understand from this?

Extract from doc: In server and hybrid (with opt-out prerender) modes, a page or API endpoint can check, set, get, and delete cookies.

👀 Current Behavior

When I set to output: hybrid in astro config file, the terminal can confusing for newbies, I know we need to opt-out of pre-render. But this warning need to be clear.

4:15:15 [WARN] `Astro.request.headers` is not available in "static" output mode. To enable header access: set `output: "server"` or `output: "hybrid"` in your config file.

🖥️ Browser

NA

📄 Additional Information

No response

sarah11918 commented 7 months ago

Thank you for raising this @jamesli2021 ! I agree and it's actually on my list to convert some of our wording over from "modes" (server, hybrid) to the actual individual pages themselves, because you can have pre-rendered modes in ANY mode, and yes, that is what the error message should be explaining. You might have one of the correct modes configured, but it will only work on a page that's rendered on-demand.

@Princesseuh - I went to look in https://github.com/withastro/astro/blob/main/packages/astro/src/core/errors/errors-data.ts for this error but I'm having trouble finding it. Is it possible we don't have a docs entry for this one?

In general, I think we'll make a task to do a sweep of SSR related errors and make sure they refer to the specific endpoint/route being prerendered or not, vs the output mode, since that doesn't guarantee that every page uses SSR.

Princesseuh commented 7 months ago

We only have pages for errors, this seems to be a random warning in the console, so it's most likely somewhere random in Astro.

sarah11918 commented 7 months ago

Alright, treasure hunt, it is! Let's find this warning in the Astro code base :sweat_smile:

mingjunlu commented 7 months ago

Gotcha!

Here's where the warning comes from: https://github.com/withastro/astro/blob/7eac778cce4429059d433fa22bf8f44555e2965f/packages/astro/src/core/request.ts#L71-L74

And here's a minimal reproduction example: https://stackblitz.com/edit/github-a8vhdv?file=src%2Fpages%2Findex.astro

lschierer commented 6 months ago

I'd really like to see this fixed, it is one of a couple of warnings that gets spewed everywhere in my console when I run a build and makes it much harder to find actual problems.

sarah11918 commented 4 months ago

I think this was already being looked into as an overly aggressive message in the first place, but that doesn't address the text of the error, so I will mention @ematipico and @matthewp here to ask whether an update to the message linked by @at-the-vr makes sense:

Instead of saying "not available in static ouput MODE", this maybe should be "this cannot be used on static prerendered PAGES" (because they may very well have server or hybrid output mode enabled, so the hint doesn't really get at the problem)? Then, the hint would be to make sure the page itself is rendered on demand, and not prerendered.

ematipico commented 4 months ago

Yeah, it makes sense to update the message

sarah11918 commented 4 months ago

It looks like the most recent version of this file has an updated error message that addresses this:

https://github.com/withastro/astro/blob/bab700d69085b1de8f03fc1b0b31651f709cbfe3/packages/astro/src/core/request.ts#L80

\Astro.request.headers` is unavailable in "static" output mode, and in prerendered pages within "hybrid" and "server" output modes. If you need access to request headers, make sure that `output` is configured as either `"server"` or `output: "hybrid"` in your config file, and that the page accessing the headers is rendered on-demand.`

So, I will close this issue since the message has in fact been updated since the original issue was filed! 🙌

firxworx commented 4 months ago

@sarah11918 I have been playing around with Astro + Lucia auth and this warning fills my console when I have the dev server running.

For auth I added a middleware check to verify that Origin + Host header of the request match for any requests where the context.request.method !== 'GET'. This the only part of the entire codebase that actually checks Astro.request.headers so I assume this is what's triggering the warning.

Astro is consistently cluttering the dev console with the warning even if I set the default output to server in my Astro config.

I agree with OP this is confusing especially because it appears to contradict a config setting that is clearly set. This makes it look like a bug in the project and risks sending the dev on a wild goose chase or it looks like a bug in Astro.

Even the "fixed" message is just as confusing -- if not worse -- given the context of this particular scenario.

I think as Astro expands more into server-side stuff more devs are going to be doing things like this and encountering this issue.

I haven't worked a ton with SSR yet with Astro so I'm not sure if in middleware there's a way to detect if the request is related to something where the server does "work" or where it doesn't and the request is for a statically generated page. It would be nice to know for conditional handling or messaging.

That said, in general, I think we still very much do want to check cookies and headers for "static" resources that are supposed to be restricted and require auth. The point of running server-side code is to check each request in case the client isn't allowed to receive the response!

ematipico commented 4 months ago

I will bring this up to the team, this is a valid concern and we need to find the correct way to address it.

ematipico commented 4 months ago

I haven't worked a ton with SSR yet with Astro so I'm not sure if in middleware there's a way to detect if the request is related to something where the server does "work" or where it doesn't and the request is for a statically generated page. It would be nice to know for conditional handling or messaging.

You can do that manually by looking at context.url, and decide to trigger the SSR logic based on that.