Removed redundant nullish coalescing operator

[chasoft]

Description (required)

This PR removes the redundant nullish coalescing operator (??) from the code that retrieves the theme from localStorage. The nullish coalescing operator is unnecessary in this context because the existence of the theme in localStorage is already checked.

Related issues & labels (optional)

• Suggested label: enhancement

[astrobot-houston]

Lunaria Status Overview

🌕 This pull request will trigger status changes.

Learn more

By default, every PR changing files present in the Lunaria configuration's files property will be considered and trigger status changes accordingly.

You can change this by adding one of the keywords present in the ignoreKeywords property in your Lunaria configuration file in the PR's title (ignoring all files) or by including a tracker directive in the merged commit's description.

Tracked Files

File	Note
en/tutorial/6-islands/2.mdx	Source changed, localizations will be marked as outdated.

Warnings reference

Icon	Description
🔄️	The source for this localization has been updated since the creation of this pull request, make sure all changes in the source have been applied.

[astrobot-houston]

Hello! Thank you for opening your first PR to Astro’s Docs! 🎉

Here’s what will happen next:

1. Our GitHub bots will run to check your changes.
   If they spot any broken links you will see some error messages on this PR.
   Don’t hesitate to ask any questions if you’re not sure what these mean!

2. In a few minutes, you’ll be able to see a preview of your changes on Netlify 🥳.

3. One or more of our maintainers will take a look and may ask you to make changes.
   We try to be responsive, but don’t worry if this takes a few days.

[netlify]

✅ Deploy Preview for astro-docs-2 ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	4645c07
🔍 Latest deploy log	https://app.netlify.com/sites/astro-docs-2/deploys/678ac253eaf4ea0008f36fc2
😎 Deploy Preview	https://deploy-preview-10719--astro-docs-2.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[ArmandPhilippot]

Hi @chasoft, and thanks for taking the time to fix the docs!

I'm a bit confused here because I remember adding that (in #10120) because Typescript was complaining without. So, I tested again to be sure it was safe to remove the nullish coalescing operator (some changes might have happen since), and it doesn't seem to be the case every time.

First I used the default branch (complete) of the blog-tutorial-demo. This branch doesn't use the nullish coalescing operator and I don't get any Typescript errors. I was surprised.

Then I remembered that we were working on the content-collections branch (and we've updated the tutorial based on that branch) so I switched. Now, src/components/ThemeIcon.astro uses the operator. If I remove it, then Typescript complains:

To be honest, I don't understand why there is a difference between the two branches. This is the same component. But since the error may be raised, I'm not sure it's safe to remove the operator from the docs...

[delucis]

Don't know if it helps but in general I'd avoid the double call to get stuff from local storage and do something like this instead:

if (typeof localStorage !== 'undefined') {
  const theme = localStorage.getItem('theme');
  if (theme) return theme; 
}

[ArmandPhilippot]

I was just confused about Typescript behavior on two files that seem identical (once the operator removed)... Refactoring didn't come to my mind. 😄

But yes, this is another solution and it might also be easier to understand while preventing Typescript errors/nullish coalescing operator.

[ArmandPhilippot]

So Fryuni has found the difference between the two branches. 🙌🏽 One uses is:inline while the other not. Same as the tutorial... I missed that, sorry. So, you're right it's safe to remove that here because Typescript will not parse that code!

But I think Chris' suggestion is a good idea to prevent the error if someone forgot the is:inline directive while following the tutorial. Do you want to make the update @chasoft?

[chasoft]

So Fryuni has found the difference between the two branches. 🙌🏽 One uses is:inline while the other not. Same as the tutorial... I missed that, sorry. So, you're right it's safe to remove that here because Typescript will not parse that code!

But I think Chris' suggestion is a good idea to prevent the error if someone forgot the is:inline directive while following the tutorial. Do you want to make the update @chasoft?

Thank you for your response. I'll take another look at this by tomorrow.

[chasoft]

hi @ArmandPhilippot,

I've just submitted a new commit to remove is:inline. You can find it here. e192c74

Reason: refactor theme retrieval from localStorage to avoid using is:inline for bypassing type checking. This approach ensures better code quality and highlights areas in our implementation that require improvement.

If this meets your approval, I'd be happy to update the demo branch at this link. https://github.com/withastro/blog-tutorial-demo/tree/87937ec6397d1a458a3bd4ec3fec49ca57b74ab2.

[delucis]

I think we do want to keep the is:inline attribute or did I misunderstand?

is:inline ensures the scripts runs where it was written instead of being bundled and executed as an async module. Bundling this script would cause a flash of unstyled content, so the is:inline usage is intentional.

[ArmandPhilippot]

Thanks @chasoft, your refactoring works with and without Typescript parsing so looks good to me!
I only left a suggestion to add back is:inline which is needed to prevent FOUC (we miss that in the content-collection branch).
After that, looks ready to merge! Welcome to Team Docs! 🚀

And you're very welcome to apply those changes to the blog-tutorial-demo. Thanks for willing to do that! 🙌🏽

[ArmandPhilippot]

Thanks for the update! LGTM!