search

withastro / docs

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

(v5) Tutorial update following template change #10025

Open ArmandPhilippot opened 1 week ago

ArmandPhilippot commented 1 week ago

📋 Explain your issue

First, sorry if I choose the wrong template... Incorrect Content Report didn't seem to fit so...

As discussed during Talking & Doc'ing yesterday (at least for me, the 14/11), the tutorial for v5 needs to be updated following the changes made by https://github.com/withastro/astro/pull/12083.

I haven't finished the tutorial yet, it took me longer than I thought. (This is the first time I do it, you have to excuse me. 🎉 Well, it's mainly taking notes that takes time! 😄 )

I stopped at Add an RSS feed. To avoid losing my notes, I preferred to share them now.

First try with pnpm create astro@beta

I tried this approach first but, I agree with Chris. If we keep pnpm create astro@latest, there are too many things to update. Even by modifying two steps while initializing the project, from step 12 it is no longer possible to follow the tutorial without major changes.

Second try with pnpm create astro@beta -- --template minimal

In summary, there are fewer changes but we will have problems with Typescript from Unit 5 onwards. I haven't looked for fixes yet, just noted the reported errors.

Also, I skipped the steps related to Github and Netlify because I don't see why they would be affected... and I didn't want to waste time with that. If needed, I'll redo the tutorial when I have more time.

  1. On Create your first Astro project
  1. On Create a blog post archive
-const allPosts = Object.values(await import.meta.glob('./posts/*.md', { eager: true }));
+const allPosts = Object.values(import.meta.glob('./posts/*.md', { eager: true }));

We don't need await. In VS Code, this is underlined with three dots when we use it: 'await' has no effect on the type of this expression.ts(80007). So it's probably best not to include it.

{allPosts.map((post) => <li><a href={post.url}>{post.frontmatter.title}</a></li>)}

Both url and frontmatter are underlined and show the following error Property 'url' does not exist on type 'unknown'.ts(2339).

  1. On Generate tag pages

First, a nit: is there an Expressive Code syntax to highlight all the added props as ins? I had seen a post on Discord about this, and by going quickly, I also got caught! It's confusing that some lines are green while some parts, also added, are gray.

Then, new Typescript error due to frontmatter in filteredPosts: Property 'frontmatter' does not exist on type 'unknown'.ts(2339).

  1. On Build a tag index page

First, a nit: same as before we don't need await for const allPosts = Object.values(await import.meta.glob('../posts/*.md', { eager: true }));

Then, again, Typescript error due to frontmatter.

  1. I stopped there, but I imagine we'll have similar problems with Typescript going forward.

I see two options:

Edit: I just checked and when using --template minimal, it seems the default Typescript preset is still "astro/tsconfigs/base". So I guess the Typescript errors I noted already exists in v4...

Consistency

  1. In the "Try it yourself - Customize your blog post layout" section of Create and pass data to a custom blog layout, we show this refactoring:
<p>Published on: {frontmatter.pubDate.toString().slice(0,10)}</p>

Then, in the next example and in the next page we show:

<p>{frontmatter.pubDate.toString().slice(0,10)}</p>

Shouldn't we keep Published on: to be consistent and avoid confusing the reader?

  1. In the "Code Check-in: MarkdownPostLayout" section of Build a tag index page
<p><em>{frontmatter.description}</em></p>
<p>{frontmatter.pubDate.toString().slice(0,10)}</p>

The description was after the publication date in the previous pages, now it is before.

Since this is a check aimed at novices, it may be better to remain consistent... (and point 1 also applies here).

Accessibility, unrelated to v5

I think we have an accessibility problem in "Test your knowledge". Firefox developer tools report me a contrast issue for:

More or less related, I would say that the green color for the checkmark stings the eyes a little... especially for the light theme. (I don't know if you say that in English... "piquer les yeux", let's just say it's not pleasant to look at 😅 )

define:vars, more or less related to v5 (v6?)

I thought there was some discussion about define:vars. Both for scripts and styles. I haven't seen any PR related to this topic, so I guess that there is no progress on this question. I preferred to note it anyway. If its use is discouraged, maybe it's time to not display it in the tutorial. Or maybe this can wait until a more precise decision...

On Style your About page, we use define:vars to define a CSS variables. Should this be updated?

sarah11918 commented 1 week ago

Ah! This is amazing! I've had my head down all day working in the files themselves, but this is all relevant in #10035 !

ArmandPhilippot commented 1 week ago

Maybe I should have mentioned you before (and maybe Chris and HiDeoo) but as I know you are busy I hesitated! 😉

sarah11918 commented 1 week ago

I thought there was some discussion about define:vars. Both for scripts and styles

Only for scripts! It's still a perfectly valid and encouraged pattern for styles!

sarah11918 commented 1 week ago

accessibility

Let's do that entirely in a different issue/PR!

sarah11918 commented 1 week ago

consistency issues

Yes, let's tackle those here!

sarah11918 commented 1 week ago

1st & 2nd try re: INSTRUCTIONS:

See my updates to the text / intro, and whether those are sufficient!

1st & 2nd try re: TYPESCRIPT ERRORS:

YES! This is what we want, and still to do in the original PR. These should absolutely be tackled here.

1st & 2nd try re: await import.meta.glob()

Let's address this issue wherever it occurs in this PR, but I distinctly remember being told to ADD IN the await (which wasn't there in the Astro.glob() version of the tutorial). So, I will need especially @delucis to weigh in here for code accuracy and pedagogy.

sarah11918 commented 1 week ago

minimal template tsconfig.json looks like strict to me. Are you on the next branch?

https://github.com/withastro/astro/blob/next/examples/minimal/tsconfig.json

ArmandPhilippot commented 1 week ago

Only for scripts! It's still a perfectly valid and encouraged pattern for styles!

Oh okay! I thought that applied to both.

accessibility

Let's do that entirely in a different issue/PR!

Yes that makes sense, I just added it there so I wouldn't forget to talk about it. 😅

1st & 2nd try re: INSTRUCTIONS:

See my updates to the text / intro, and whether those are sufficient!

From what I've read, I think so! But I'm up for retesting the tutorial with these changes in mind and going all the way through this time. I can do this tomorrow, rather in the afternoon (for me), so within 24 hours I should be able to add more information.

minimal template tsconfig.json looks like strict to me. Are you on the next branch?

https://github.com/withastro/astro/blob/next/examples/minimal/tsconfig.json

Following the link, you are right. However, when I use pnpm create astro@beta -- --template minimal (I just checked again), the tsconfig.json file contains:

{
  "extends": "astro/tsconfigs/base"
}

Maybe the astro@beta package is not up to date?

sarah11918 commented 1 week ago

Maybe the astro@beta package is not up to date?

Interesting! So for the sake of THIS PR, assume that it will be the strict template.

This is what we currently show for running create astro for beta 5. Does it work with this command?

image

ArmandPhilippot commented 1 week ago

Oh ok, so I followed the tutorial with Astro v4 apparently... I didn't think to check package.json. 😭 I did try @next and it didn't work at all. Then, I though @beta would work with v5...

So I just tested both pnpm create astro@latest -- --ref next and pnpm create astro@latest -- --ref next --template minimal. Both works with v5.

With pnpm create astro@latest -- --ref next, we still have How would you like to start your new project? and I can choose Empty (which seems to be minimal).

And in both cases, I have the following prompt: How strict should TypeScript be? and I can choose Relaxed (so, base).

I'm even more confused now. I thought the PR removed these questions... This definitely needs more testing...

Edit: And even more confused since those questions was not present in my tests with v4... 👀 Edit2: With astro@beta not only were the questions no longer present but the template matched (that's why I tough it was v5...). I didn't checked the default template with astro@latest -- --ref next but it seems strange to me that the questions are there since the PR is merged...

sarah11918 commented 1 week ago

Uh, well... I'm not sure what to expect from all that! Might be time to ask the devs what's going on there!

ArmandPhilippot commented 1 week ago

I just retested both commands to make sure I didn't do something stupid last night. 😅 I put a more detailed "report" below... but it corresponds to my observations from yesterday (except I though that astro@beta uses the Welcome component... it does not).

I didn't test with --template minimal. I wanted to see the differences with the default choices.

So, pnpm create astro@latest -- --ref next seems to be the right command, only the prompt does not match...

pnpm create astro@latest -- --ref next

  "dependencies": {
    "astro": "^5.0.0-beta.8",
    "@astrojs/check": "^0.9.4",
    "typescript": "^5.6.3"
  },
Prompt Template
astro-tutorial-astro-latest-ref-next-prompt astro-tutorial-astro-latest-ref-next-template

pnpm create astro@beta

  "dependencies": {
    "astro": "^4.16.13"
  },
Prompt Template
astro-tutorial-astro-beta-prompt astro-tutorial-astro-beta-template
ArmandPhilippot commented 1 week ago

So I redid the tutorial, in full this time (except Github/Netlify). I relied on #10035.

Other than what I've already noted, there's not much new. Typescript errors are just a little different (and again, the principle is the same: it doesn't know the shape of objects).

  1. Create your first Astro project

Here, given the previous observation, I did not follow the steps to the letter but your modifications seem good to me. But, if we want to be faithful to the questions asked, nit:

Here my steps:

  1. Use your first CSS variable

This is not really a change, rather a "bug". Using define:vars on <style>, the HMR does not pick up this change automatically. I first though it was due to the "Auto Save" feature in VSCode but the next time with fontWeight/textCase I saved manually and the result was the same. In both cases I had to save twice to get the variables set (other changes were taken into account).

I'm not sure what to do here. Should we add a hint that if the changes are not taken into account we should try saving again?

  1. Write your first script tag

Typescript shows an issue with Step 1:

Object is possibly 'null'.ts(2531)

So we need to update the code snippet (?.)):

```astro title="src/pages/index.astro" ins={2-6}
  <Footer />
  <script>
    document.querySelector('.hamburger')?.addEventListener('click', () => {
      document.querySelector('.nav-links')?.classList.toggle('expanded');
    });
  </script>
</body>

To be consistent we also need to update Step 2 in [Importing a .js file](https://deploy-preview-10035--astro-docs-2.netlify.app/en/tutorial/3-components/4/#importing-a-js-file):

````md
```astro title="src/pages/index.astro" ins={7} del={3-5}
  <Footer />
  <script>
    document.querySelector('.hamburger')?.addEventListener('click', () => {
      document.querySelector('.nav-links')?.classList.toggle('expanded');
    });

    import "../scripts/menu.js";
  </script>
</body>

4. [Dynamically display a list of posts](https://deploy-preview-10035--astro-docs-2.netlify.app/en/tutorial/5-astro-api/1/#dynamically-display-a-list-of-posts)

Typescript shows an issue with Step 2:

'post' is of type 'unknown'.ts(18046)


It seems the error is slightly different with `strict` if we compare with my notes in the first post (the issues were `url` and `frontmatter`).

We also have this issue in:
* the step 5 in [Challenge: Create a BlogPost component](https://deploy-preview-10035--astro-docs-2.netlify.app/en/tutorial/5-astro-api/1/#challenge-create-a-blogpost-component)
* the steps 2, 3 and 4 in [Use props in dynamic routes](https://deploy-preview-10035--astro-docs-2.netlify.app/en/tutorial/5-astro-api/2/#use-props-in-dynamic-routes)
* the steps 2, 3 and 4 in [Advanced JavaScript: Generate pages from existing tags](https://deploy-preview-10035--astro-docs-2.netlify.app/en/tutorial/5-astro-api/2/#advanced-javascript-generate-pages-from-existing-tags)
* the step 2 in [Create an array of tags](https://deploy-preview-10035--astro-docs-2.netlify.app/en/tutorial/5-astro-api/3/#create-an-array-of-tags)

5. [Challenge: Include tags in your blog post layout](https://deploy-preview-10035--astro-docs-2.netlify.app/en/tutorial/5-astro-api/3/#challenge-include-tags-in-your-blog-post-layout)

Typescript shows a new error due to `frontmatter.tags.map((tag)`:

Parameter 'tag' implicitly has an 'any' type.ts(7006)



6. [Create a collection for your posts](https://deploy-preview-10035--astro-docs-2.netlify.app/en/tutorial/7-collections/#create-a-collection-for-your-posts)

Just a nit regarding the Step 4:

* When we restart the dev server, `astro sync` is automatically called. So `npx astro sync` is not needed. But we can leave it to say that the command exists, it does no harm!
* Also, until now, when there was a command we used `PackageManagerTabs`. Maybe we should do it here too.

7. [Generate pages from a collection](https://deploy-preview-10035--astro-docs-2.netlify.app/en/tutorial/7-collections/#generate-pages-from-a-collection)

We miss an import (`render`) in Steps 2 and 3.

8. [Update any frontmatter values to match your schema](https://deploy-preview-10035--astro-docs-2.netlify.app/en/tutorial/7-collections/)

We say to use `toString()` for the publication date, but this already what we have in the previous steps (`frontmatter.pubDate.toString().slice(0,10)`).

If the idea is to show that there may be changes to be made, perhaps we could use: `frontmatter.pubDate.toLocaleDateString()`?

____

Typescript shows errors, but everything works!
Using `"astro/tsconfigs/base"` in the Typescript config removes some errors but new ones appear (like in my first post)...

I noticed another difference between Erika's PR and `pnpm create astro@latest -- --ref next`, in package.json: `"build": "astro check && astro build"` should be `"build": "astro build"`... (it is the case with `astro@beta`...).
sarah11918 commented 1 week ago

Amazing, thank you @ArmandPhilippot !!

My thoughts:

1.

2.

  1. , 4. , 5.

7.

8.

I'm a little confused because I don't see this previously on this page? This should be the step where toString() is introduce as in the previous code the date was a string. I'm not sure what change is needed here? I think this step is the minimal, and clearest way to point out going from a string to not a string.

As for any issues with what create astro now does vs what we thought it does/what the PR changed it to do, seems like maybe that should be an issue for the astro repo?

sarah11918 commented 1 week ago

Noting that there is an open PR #10035 and we would love help implementing the changes that Armand has laid out and I have responded to above! :raised_hands:

ArmandPhilippot commented 1 week ago
  1. , 4. , 5.
* Thank you for finding! Please do suggest any and all updates to code snippets necessary! That is the main point of this PR! 🙌

The point 3 was easy to fix. However for 4 and 5... I'm not sure what is best. For 4. (at least for the first problematic code snippet), it seems that using post: any removes the error. For 5., using tag: string also removes the error. For example:

{
    frontmatter.tags.map((tag: string) => (
      <p class="tag">
        <a href={`/tags/${tag}`}>{tag}</a>
      </p>
    ))
  }

But it doesn't seem to me that we've already defined the type of a variable before, so isn't that likely to be confusing for a beginner?

* If `npx astro sync` is not required then let's remove it entirely. The tutorial is for walking through minimum steps.

* Yes, agree to add package manager tabs

Each time we restart the Dev Server, astro sync is executed internally: [content] Syncing content. So something like the sentence below should be sufficient:

In order for Astro to recognize your schema, quit (CTRL + C) and restart the dev server to continue with the tutorial. This will define the astro:content module.

The alternative would be to explain that you need to open another terminal in VSCode, and run the astro sync command. But for a beginner, stop/restart seems simpler to me.

And without this sentence, the PackageManagerTabs is no longer required.

I'm a little confused because I don't see this previously on this page? This should be the step where toString() is introduce as in the previous code the date was a string. I'm not sure what change is needed here? I think this step is the minimal, and clearest way to point out going from a string to not a string.

Indeed, on this page we do not use .toString() anywhere other than in this section. Sorry if I wasn't clear enough... I also realize that I forgot the direct link for this section. 😅

toString() is introduced much earlier! The first occurrence is in unit 4, in the Try it yourself - Customize your blog post layout section. The first src/layouts/MarkdownPostLayout.astro code snippet already shows:

---
const { frontmatter } = Astro.props;
---
<h1>{frontmatter.title}</h1>
<p>Published on: {frontmatter.pubDate.toString().slice(0,10)}</p>
<p>Written by {frontmatter.author}</p>
<slot />

If I'm not mistaken, the last modification of MarkdownPostLayout is in Nest your two layouts. As you can see, we still have:

<p>{frontmatter.pubDate.toString().slice(0,10)}</p>

So what we are proposing in Update any frontmatter values to match your schema does not seem to me to change anything. The code snippet tells us to insert .toString() which is already there. This is why I am confused and suggested changing the format.

Thanks to the content collection, pubDate is now inferred as a date, so we can use a native Date method to display it. I think using toLocaleDateString() is still a minimal example and it shows that now that the type is correctly inferred, we can take advantage of native methods to format our date if needed.

Don't hesitate if I haven't managed to be clearer... 😅

As for any issues with what create astro now does vs what we thought it does/what the PR changed it to do, seems like maybe that should be an issue for the astro repo?

Oh okay, I thought you might be able to get some internal info on this but an issue works too!

ArmandPhilippot commented 1 week ago

Regarding create astro, before opening an issue I might have a lead on why the changes from Erika's PR are not all there!

I just checked the [ci] release PR and, I see that:

But I don't know how to proceed to test the beta version of create-astro...

Edit: Nevermind I finally understood how to combine both:

pnpm create astro@beta -- --ref next --template minimal

🎉

For the explanation, I thought @beta controlled the version of Astro while it controls the version of create-astro... This is why the two commands brought different changes... Together we have all the PR changes!

Edit2: The styles still seems broken but I saw a new PR related to the Welcome component so I don't think we need to create an issue in withastro/astro for now! Everything else is as it should be with this command.

sarah11918 commented 6 days ago

OK, I think this conversation is now entirely moved to the two open PRs:

10049 with Armand's suggestions that will merge into #10035 (where more edits/reviews can still happen!)

For ease of keeping track of things, let's try to mostly comment in PRs right now rather than this issue! 🙌