Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(v2): feature/docs slug frontmatter (v2) #2771

Merged
merged 10 commits into from
Jun 2, 2020

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented May 18, 2020

Motivation

Solve #2697

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Jest tests

@slorber slorber requested a review from yangshun as a code owner May 18, 2020 17:47
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label May 18, 2020
@@ -79,7 +79,7 @@ describe('simple site', () => {

expect(data).toEqual({
id: 'foo/baz',
permalink: '/docs/foo/baz',
permalink: '/docs/any/pathname.html', // pathname is not "affected" by markdown folder
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if a markdown file is in a subfolder, not sure we want to force the user to have a pathname prefixed by that folder's name. He may group files logically by folder, but still want custom pathnames? don't know

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented May 18, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 5fd47be

https://deploy-preview-2771--docusaurus-2.netlify.app

@slorber
Copy link
Collaborator Author

slorber commented May 18, 2020

Hi @JoelMarcey @yangshun
This is a POC for customizing the pathname of a doc.

I think it's more "clean" than the workaround of having id: nodejs.html, as it does not require the user to have sidebar.js reference documents with html extension as id.

What I wonder is, how much freedom should we give to the user regarding this customization.

If a doc is in /versioned_docs/version-2.0.0-alpha.50/subfolder/someDoc.md

The url would be by default: docs/2.0.0-alpha.50/subfolders/someDoc

Using pathname: subpath/someDoc.html frontmatter could produce those different possible routes:

  1. docs/2.0.0-alpha.50/subfolders/subpath/someDoc.html
    would constrait the user to a subpath of the folder's name

  2. docs/2.0.0-alpha.50/subpath/someDoc.html
    would allow to escape the FS hierarchy and customize the math

  3. subpath/someDoc.html
    would be confusing regarding versioning, but allow the most flexibility


What I think we should do for now is probably keep it simple for the Watchman usecase:

  • pathname should not contain /
  • pathname, use the folder's name as path prefix (like with id)

If there's more need, we can still build a generic hook to be able to rewrite pathnames before generating the final routes. In such case the user would have full control, somehow like:

const customizeRoutePathname = (route) => {
  if ( route.pathname === "/docs/2.0.0-alpha.50/subfolders/someDoc" ) {
    return pathname + ".html
  }
  return pathname;
}

As I don't really know how common is this need, wonder if it's worth investing too much

@JoelMarcey
Copy link
Contributor

As I don't really know how common is this need, wonder if it's worth investing too much

I agree. We have not seen a huge ask for this fix for v1 sites. I say we start simple with the Watchman fix - which probably fixes some other affected sites too - and see if we have to do any further work.

@slorber
Copy link
Collaborator Author

slorber commented May 20, 2020

Hi,

I've decided to rename the frontmatter "slug", and only affect the last section of the pathname (as an user would expect from a slug).

I think it's the most simple, intuitive, minimal api surface to solve the watchman problem.

Ready to review.

@slorber slorber changed the title Feature/docs pathname frontmatter Feature/docs slug frontmatter (v2) May 20, 2020
@yangshun yangshun changed the title Feature/docs slug frontmatter (v2) feat(v2): feature/docs slug frontmatter (v2) May 22, 2020
@yangshun yangshun requested a review from lex111 May 22, 2020 21:06
@yangshun
Copy link
Contributor

@lex111 what do you think about this PR? Since you're more familiar with the docs plugin now.

Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

Should we mention about this feature on "Migration from v1 to v2" doc?

For example, in v1 the cleanUrl option may be disabled, so we could update the following sentence on that doc page, like so:

- cleanUrl - Clean URL is used by default now.
+ cleanUrl - Clean URL is used by default now. However you can specify slug to add HTML extension to your URLs.

@lex111 lex111 added this to the v2.0.0-alpha.57 milestone May 27, 2020
@slorber
Copy link
Collaborator Author

slorber commented May 28, 2020

Hi @lex111

Thanks for telling me about clean urls.

As far as I understand, in v1, we could access doc at /doc and /doc.html, and there was no canonical url.

For the migration guide, I'd recommend the user to choose between /doc or /doc.html is the canonical url, and create a client side redirect from the non-caninocal url with the client side redirect plugin of #2793.

If this is really needed, we could write the same page twice to the FS and add the same canonical url to both pages, that would reproduce the behavior of v1, but it's not something I've worked on yet and I think the client-side redirect should be enough.

I'd rather document the migration in the client side redirect plugin PR, what do you think?

Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

Just doc improvement and I think we ready to merge.

website/docs/docs.md Outdated Show resolved Hide resolved
Co-authored-by: Alexey Pyltsyn <[email protected]>
@slorber
Copy link
Collaborator Author

slorber commented Jun 1, 2020

change applied to the doc, thanks

@slorber
Copy link
Collaborator Author

slorber commented Jun 1, 2020

Was wondering if we should also support this feature for the blog posts (front matter) and pages (comp static value) too?

If v1 users have cleanUrl: false they would all have /path.html extension in the url bar (/path still works, and there's no canonical url metadata).

For example, if D1 site had cleanUrl: false, it's possible that this link ends up being copied somewhere and indexed by google: https://docusaurus.io/en/users.html
For D1 users migrating to D2, should we allow them to keep /users.html as the canonical url? Currently it's not really possible.

That's likely a niche usecase.
I don't know how much D1 sites have cleanUrl: false and care much about SEO.

I think we should merge this PR anyway, and improve if some user complains during a migration.

Note: Watchman is not coming from D1 and not affected.

Edit: the non-migrated D1 sites of the spreadsheet that seems to use cleanUrl: false are:

@lex111
Copy link
Contributor

lex111 commented Jun 2, 2020

Was wondering if we should also support this feature for the blog posts (front matter) and pages (comp static value) too?
Currently it's not really possible.

I think we should not do this, especially when there is a official redirect plugin, the end user can easily add needed redirects, right?

@slorber
Copy link
Collaborator Author

slorber commented Jun 2, 2020

Agree

We could merge this PR, and see later if one of the 4 sites with cleanUrl: false ask for blogs/pages having html extensions for their canonical urls. It's not worth to add a feature if nobody use it in the end.

@lex111 lex111 merged commit b8de9c6 into facebook:master Jun 2, 2020
@lex111
Copy link
Contributor

lex111 commented Jun 2, 2020

@slorber thank you!

@slorber slorber mentioned this pull request Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants