Skip to content

feat: astro:env stable#9160

Merged
florian-lefebvre merged 21 commits into
5.0.0-betafrom
feat/astro-env-stable
Sep 6, 2024
Merged

feat: astro:env stable#9160
florian-lefebvre merged 21 commits into
5.0.0-betafrom
feat/astro-env-stable

Conversation

@florian-lefebvre
Copy link
Copy Markdown
Member

@florian-lefebvre florian-lefebvre commented Aug 21, 2024

http://localhost:4321/en/guides/environment-variables/

Description (required)

Related issues & labels (optional)

  • Closes #
  • Suggested label:

@netlify
Copy link
Copy Markdown

netlify Bot commented Aug 21, 2024

Deploy Preview for astro-docs-2 ready!

Name Link
🔨 Latest commit 97ba73c
🔍 Latest deploy log https://app.netlify.com/sites/astro-docs-2/deploys/66d9c9aeb28489000825790b
😎 Deploy Preview https://deploy-preview-9160--astro-docs-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@sarah11918 sarah11918 added this to the 5.0.0-beta milestone Aug 21, 2024
Copy link
Copy Markdown
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick initial review.. I didn't realized until I opened this up that it wasn't exactly ready for a review yet. 😅

See my comment below re: just reusing the material we already have, then working from there!

Comment thread src/content/docs/en/guides/upgrade-to/v5.mdx Outdated
Comment thread src/content/docs/en/guides/upgrade-to/v5.mdx Outdated
Comment thread src/content/docs/en/guides/environment-variables.mdx Outdated
florian-lefebvre and others added 2 commits August 22, 2024 14:11
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@florian-lefebvre
Copy link
Copy Markdown
Member Author

@sarah11918 I added a bunch of content but I'm really unsure about how to structure things, would love your guidance!

Comment thread src/content/docs/en/guides/environment-variables.mdx Outdated
Comment thread src/content/docs/en/guides/environment-variables.mdx
@sarah11918
Copy link
Copy Markdown
Member

Hey @florian-lefebvre , I'll be looking at this more closely today, but just pointing out that some of this content should end up in reference pages and NOT crammed all in here, which is more of a guide page.

As an example, see what I'm suggesting in #9133 - the guide content doesn't have to spell things out like an API reference (and shouldn't, because then we're duplicating content and our work!). Rather, it tells the story around using this and links to references where all the details of the items are kept.

So that's my first piece of advice, keep this for people learning about/setting this up for the first time, or coming back to confirm general usage, not looking up values etc. And then we'll link to each item's reference location if they need to e.g. confirm types etc.

@florian-lefebvre
Copy link
Copy Markdown
Member Author

@sarah11918 tried my best to improve things!

@sarah11918
Copy link
Copy Markdown
Member

Just noting that I removed changes from the upgrade guide because I've already been working on that separately! This should also resolve conflicts on the environment variables page, so we can start fresh with that one!

@sarah11918
Copy link
Copy Markdown
Member

Hey @florian-lefebvre ! I committed an editing pass here that I think might be a more helpful organization structure? For example, the stuff about how to actually set values for environment variables was inside "Support for Vite" and you're gonna need to set these variables when using astro:env, too! 😄

So I tried to envision a flow for the entire page that is like;

  • you have full Vite support, and some included defaults you can just use out of the box
  • here's how you set them
  • here's how you retrieve them
  • Here's how you get type safe environment variables
  • Intellisense (since it relates to both)

getSecret() absolutely needs an API reference entry on the API reference page. Is there anything else you'd put there with it? This section of this page is a little less fleshed-out, so let's start by getting a good API reference for this one, then we'll know what's already been said that we can just link to from this page. And this page can focus more on using it. E.g. maybe a better heading than getSecret for this page (since it's not the API reference) is "Retrieving secrets dynamically".

So see what you think about this, and tell me where / how it doesn't work so we can iterate on it!

@florian-lefebvre
Copy link
Copy Markdown
Member Author

@sarah11918

  • I moved ts intellisense under vite support since it's not relevant for astro:env
  • i moved getSecret to the api reference
  • i still don't know what to do with the limitations, i don't know if there are similar cases in docs?

Comment thread src/content/docs/en/guides/environment-variables.mdx Outdated
Comment thread src/content/docs/en/reference/api-reference.mdx Outdated
Comment thread src/content/docs/en/reference/api-reference.mdx Outdated
Comment thread src/content/docs/en/reference/api-reference.mdx Outdated
Comment thread src/content/docs/en/reference/api-reference.mdx Outdated
Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>
Comment thread src/content/docs/en/reference/api-reference.mdx Outdated
Comment thread src/content/docs/en/reference/adapter-reference.mdx Outdated
Comment thread src/content/docs/en/reference/adapter-reference.mdx Outdated
florian-lefebvre and others added 4 commits September 5, 2024 15:01
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Comment thread src/content/docs/en/reference/api-reference.mdx Outdated
Copy link
Copy Markdown
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're finally happy with this????

Comment thread src/content/docs/en/reference/adapter-reference.mdx
Comment thread src/content/docs/en/reference/adapter-reference.mdx Outdated
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@florian-lefebvre florian-lefebvre marked this pull request as ready for review September 5, 2024 14:30
Comment thread src/content/docs/en/guides/environment-variables.mdx Outdated
Comment thread src/content/docs/en/guides/environment-variables.mdx
Comment thread src/content/docs/en/guides/environment-variables.mdx Outdated
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Comment thread src/content/docs/en/guides/environment-variables.mdx Outdated
Copy link
Copy Markdown
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, realizing this can be merged now because the link to #envschema shouldn't break! It just won't have the information promised to be there yet. until the core PR is merged. 😄

@sarah11918 sarah11918 added the Merge Queue Approved and ready to be merged (wait for feature release if also labelled M-O-R)! label Sep 5, 2024
@florian-lefebvre florian-lefebvre merged commit 182b5f3 into 5.0.0-beta Sep 6, 2024
@florian-lefebvre florian-lefebvre deleted the feat/astro-env-stable branch September 6, 2024 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5.0.0-beta Merge Queue Approved and ready to be merged (wait for feature release if also labelled M-O-R)!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants