On-demand Rendering Adapters @ Cookies examples are not working - Correct implementation provided

Kodalem commented 4 months ago

📚 Subject area/topic

On-demand Rendering Adapters @ Cookies

📋 Page(s) affected (or suggested, for new content)

https://docs.astro.build/en/guides/server-side-rendering/#cookies https://docs.astro.build/en/reference/api-reference/#set

📋 Description of content that is out-of-date or incorrect

Given example does not work anymore and is likely due to being outdated, because the Astro.cookies.set("counter",counter) accepts types key: string, value: string | Record<string, any>, options?: AstroCookieSetOptions instead of (key: string, value: string | number | boolean | object, options?: CookieSetOptions) => void. Example works only for npm run dev, but fails with npm run build with TypeScript error 2345.

Also to note, there's also TypeScript error 2532, as the cookie value in const cookie = Astro.cookies.get("counter") might be undefined and fixed with !. parameter like so const cookie = Astro.cookies!.get("counter").

Broken example:

---
let counter = 0

if (Astro.cookies.has("counter")) {
  const cookie = Astro.cookies.get("counter")
  counter = cookie.number() + 1
}

Astro.cookies.set("counter",counter)
---
<html>
  <h1>Counter = {counter}</h1>
</html>

Working basic example using Record type, compiles without error nor warnings.

---
// Create the Record Type
interface CookieData {
   exampleString: string;
   counter: number;
}
// Create the Example Data
let CookieDataExample: CookieData = {
   exampleString: "example",
   counter: 0,
}
let counterFromCookie = -1

if (Astro.cookies.has("counter")) {
    console.log("cookie exists")
    const cookie = Astro.cookies.get("counter")
    // Parse as JSON data and get the counter value
    console.log(cookie!.json())
    counterFromCookie = cookie!.json().counter
    CookieDataExample.counter = counterFromCookie + 1
}
Astro.cookies.set("counter", CookieDataExample)
---
<html>
  <h1>Counter = {CookieDataExample.counter}</h1>
</html>

🖥️ Reproduction in StackBlitz (if reporting incorrect content or code samples)

No response

Kodalem commented 4 months ago

Also apologies if not using reproduction in StackBlitz, as I am new to web development for hobby purposes!

Line let counterFromCookie = -1 is used as an definer, but -1 value for debugging and educational line to note that the implementation uses the cookies' value instead of the local variable.

sarah11918 commented 3 months ago

Thank you for reporting an issue with our examples, @Kodalem ! I am labeling this with confirm behaviour to encourage others to attempt to reproduce both the issue and your proposed solution!

Would love to have some confirmation from others who have reproduced so we can make sure we have correct examples in the docs!

HiDeoo commented 1 month ago

The current example does indeed contain a few TypeScript errors that could prevent a default build that would run astro check before the build. Here is a preview of the errors:

Regarding the proposed solution, even though it fixes the TypeScript errors:

It makes a few assumptions about the cookie value that may not be true in all cases. e.g. there is no garantee that the cookie value will be valid JSON and in that case, json() would throw an error, and if it is valid JSON, there is also no garantee that the counter key will be present in the object and be a number.
I personally feel like the example is a bit too complex with a new TypeScript interface compared to the number example that was previously used.

I think a simpler approach could be to preserve the current example and just fix potential errors. I think this could look like something like this (the comments are added to explain the changes):

---
let counter = 0

if (Astro.cookies.has('counter')) {
    const cookie = Astro.cookies.get('counter')
    // Get the number value from the cookie.
    const value = cookie?.number()
    // If the value is defined and is not `NaN`, use it as the counter and increment it.
    if (value !== undefined && !isNaN(value)) counter = value + 1
}

// Set the new value in the cookie by converting it to a string.
Astro.cookies.set('counter', String(counter))
---
<html>
  <h1>Counter = {counter}</h1>
</html>

Do you think this could be a good alternative?

Fryuni commented 1 month ago

It doesn't even need the if (Astro.cookies.has('counter')) block anymore. If it doesn't have the cookie it will return undefined and also work.

HiDeoo commented 1 month ago

It doesn't even need the if (Astro.cookies.has('counter')) block anymore. If it doesn't have the cookie it will return undefined and also work.

Totally correct, I assumed (and maybe I'm wrong) that the idea was to show has, get and set in one example that is very explicit about what is happening. Curious to know everyone's opinion on this, e.g. should this be simplified even more or the current logic helps understanding the example better?

sarah11918 commented 1 month ago

Thanks for jumping in everyone!

I think I agree with Hideoo that we can just update the existing example, even if more verbose than necessary, because it does illustrate more in one compact example!

So to be clear, I think we're agreeing the intended fix is to the code example located here: https://docs.astro.build/en/guides/server-side-rendering/#cookies

And replace it with:

---
let counter = 0

if (Astro.cookies.has('counter')) {
    const cookie = Astro.cookies.get('counter')
    const value = cookie?.number()
    if (value !== undefined && !isNaN(value)) counter = value + 1
}

Astro.cookies.set('counter', String(counter))
---
<html>
  <h1>Counter = {counter}</h1>
</html>

If we're all happy, then I'd welcome a PR to update that example!

Bonus points, it should also be added to the same place in the new on-demand rendering page in the beta branch (5.0.0-beta), too! https://5-0-0-beta.docs.astro.build/en/guides/on-demand-rendering/#cookies

Kodalem commented 1 month ago

Hmm, I need to check why this new example still throws the TS2345 with the proposed merged example. It demands explicit type setting.

Is it because I am in the strictest TypeScript mode that it demands doing interfaces? I will try to later make a separate virgin Astro setup with strict and base mode.

Although I don't see why interfaces would to be too complex, because it is fundamentally similar to C, C++ and Rust, but it maybe due to my bias of having come from low-level programming - I was quite surprised that it is very opaque in the documentation that interfaces exist in many things that are hidden away unless you look under the hood. I can see why it would maybe confuse who come from somewhere else (referencing https://xkcd.com/2501/)

I hope Google will cache this issue in the searches, because if it may not be in the documentation - it may help someone else who has, I assume unwanted, level of explicitness desired when they try to build it.

withastro / docs