facebook / docusaurus

Easy to maintain open source documentation websites.
https://docusaurus.io
MIT License
56.62k stars 8.51k forks source link

Early error if a blog post has slug: '/' that would be shadowed by home page #7870

Open MarkShawn2020 opened 2 years ago

MarkShawn2020 commented 2 years ago

Have you read the Contributing Guidelines on issues?

Prerequisites

Description

A md file with frontmatter with slug of / woule generate path as /blog/ by default.

But actually, /blog/ would redirect to be /blog, i.e. the home page of blogs.

Steps to reproduce

place a md file under blog dir, i.e. /blog/test.md:

---
slug: /
---

# test

Expected behavior

Actual behavior

  1. /blog or /blog/ all points to the home page of blog
  2. but the blog page (containing 5 articles default) would have the right arcticle rendered from /blog/test.md
  3. if the arcticle with slug of / failed to be parsed at the same time (e.g. <div style="color: red">xxx</div> is invalid since the valid jsx grammar is style={{color: "red"}}.), then the build engine would not report the error directly from this arcticle, but from the page(containing of 5 arcticles defualt), which is very confusing to the developers (e.g. me).
    • I do not know well now that the md would be finally rendered, why the render part of /blog/ should be just skipped ...

Your environment

Self-service

Josh-Cena commented 2 years ago

I think we should refuse any post slug that may overwrite a "built-in page", like the list pages, archive page, tags page, etc. Not sure how easy that is.

Do you get duplicate routes warnings?

MarkShawn2020 commented 2 years ago

I think we should refuse any post slug that may overwrite a "built-in page", like the list pages, archive page, tags page, etc. Not sure how easy that is.

Do you get duplicate routes warnings?

From my test, no duplicated routes would be warned, unless you explictly write two or more files with the same slug.

MarkShawn2020 commented 2 years ago

I think we should refuse any post slug that may overwrite a "built-in page", like the list pages, archive page, tags page, etc. Not sure how easy that is.

Do you get duplicate routes warnings?

see packages/docusaurus-plugin-content-blog/src/frontMatter.ts:66, there is a joi config for field of slug.

Josh-Cena commented 2 years ago

If we want to fix this, we would have to fix shadowing of the /archive page, /tags page, /page/2 page, etc. as well. That would require the front matter validator to know about what built-in pages would be generated.

Otherwise, we could do it at the createPage phase, instead of validation. That should work the same.

MarkShawn2020 commented 2 years ago

If we want to fix this, we would have to fix shadowing of the /archive page, /tags page, /page/2 page, etc. as well. That would require the front matter validator to know about what built-in pages would be generated.

Otherwise, we could do it at the createPage phase, instead of validation. That should work the same.

From my opinion, to implement the logic within createPage may be more reasonable, let the joi itself focus more on the grammar domain, rather than the business one.

slorber commented 2 years ago

Do we really always want the 1st page to be the home?

Shouldn't we allow customizing this pagination path logic?

  function permalink(page: number) {
    return page > 0
      ? normalizeUrl([basePageUrl, `page/${page + 1}`])
      : basePageUrl;
  }

Doesn't it make sense to replace the default homepage of your blog with the archive page that you swizzle to display it nicely? I could want this.


I'm not fan of a specific solution using frontmatter validation

hink we should refuse any post slug that may overwrite a "built-in page", like the list pages, archive page, tags page, etc. Not sure how easy that is. Do you get duplicate routes warnings?

From my test, no duplicated routes would be warned, unless you explicitly write two or more files with the same slug.

I just wrote slug: / in a blog post and got a warning 🤷‍♂️ you can make it throw with config.

CleanShot 2022-08-09 at 14 03 40@2x

Isn't it good enough? Can we make it more visible, and the wording easier to understand?

Josh-Cena commented 2 years ago

Yes—we do get a warning, but the message is kind of vague. Within the realm of a plugin we can give a much more sensible diagnostic. I think that's the point of this issue.