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

fix: frontmatter description duplication (#194) #170

Merged
merged 6 commits into from
Feb 9, 2021
Merged

fix: frontmatter description duplication (#194) #170

merged 6 commits into from
Feb 9, 2021

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Dec 1, 2020

fix #170

Support description in frontmatter, removing another diff with vuepress and giving an easy way to define the description of each page that is really common (instead of going through head/meta options)

If both description and head meta description are present in frontmatter, the description option takes priority.

The PR also removes the meta description from the frontmatter head, because it is already used to infer the pageData.description field. Because description meta is treated separately from head processing in render.ts and head.ts, there were two meta description tags in the HTML head.

I first directly filtered out the meta description from the frontmatter head, because I think that it is less error prone but users may find strange that it is gone if they try to access it from the .md.

So in the end the PR is filtering out the description directly where the head is used, both in render.ts and in head.ts. I tried to create a @shared/head file for these helpers but got compilation errors that I did not know how to fix, so they at this point rejectHeadDescription is duplicated in both files.

The same double meta description is still occurring if there is a description in siteData head. This PR doesn't include a fix for that case. I think something similar should be done, in resolveSiteData the description should check if there is a meta description in siteData.head before defaulting to 'A Vitepress site' and infer it from there too. Then in render.ts and head.ts we can use the same rejectHeadDescription of siteData.head

There could also be duplication with meta viewport that is treated specially as with description. The same scheme could be applied to siteData and pageData for it.

)
}

function rejectHeadDescription(head: HeadConfig[]) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can move these functions to src/shared so we don't duplicate the implementation across node and client?

Also, thinking maybe filterHeadDescription could be more clear?

Copy link
Member Author

Choose a reason for hiding this comment

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

Totally agree that needs to be moved to shared, but I tried to create a @shared/head file for these helpers before sending the PR but got compilation errors that I did not know how to fix.

About filterHeadDescription, at least for me, it sounds that you are going to get only the head description (instead of filtering it out). I associate it with the positive check condition like filterApples === filter( _ , isApple ). I used reject as in https://lodash.com/docs/4.17.15#reject. But not an issue if that is changed 👍🏼

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried again to move rejectHeadDescription to shared/config.ts, importing it as ../shared/config from node and /@shared/config from client. This is the error I do not know how to resolve:

λ yarn docs-dev
yarn run v1.22.10
$ node ./bin/vitepress dev docs
vitepress v0.9.0
vite v1.0.0-rc.13
internal/modules/cjs/loader.js:1032
  throw err;
  ^

Error: Cannot find module '../dist/node'
Require stack:
- C:\Labs\vitepress\bin\vitepress.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:1029:15)
    at Function.Module._load (internal/modules/cjs/loader.js:898:27)
    at Module.require (internal/modules/cjs/loader.js:1089:19)
    at require (internal/modules/cjs/helpers.js:73:18)
    at Object.<anonymous> (C:\Labs\vitepress\bin\vitepress.js:16:3)
    at Module._compile (internal/modules/cjs/loader.js:1200:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1220:10)
    at Module.load (internal/modules/cjs/loader.js:1049:32)
    at Function.Module._load (internal/modules/cjs/loader.js:937:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [ 'C:\\Labs\\vitepress\\bin\\vitepress.js' ]
}
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

FWIW, I think the structure of shared code in Vitepress may change quite a bit once Vitepress is migrated to Vite 2 and this kind of helpers would end up in a rollup plugin shared by both dev and build processes

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the late check in! Do you have any update on this one? @matias-capeletto 👀 Well Vite 2 here, and maybe we could tackle rollup plugin approach, though I think that's out of this PR's scope.

So maybe renaming method name and lets merge this...?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think if this is going to move to a plugin, the redundancy will be shortlived anyways.
I renamed reject to filterOut, I hope that is clearer.
I also merged master into the branch and everything still looks good.

@vercel
Copy link

vercel bot commented Jan 3, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/vuejs/vitepress/7460nc7tz
✅ Preview: https://vitepress-git-fork-matias-capeletto-feat-frontmatte-64ae40.vuejs1.vercel.app

@kiaking kiaking added the enhancement New feature or request label Jan 22, 2021
@kiaking kiaking changed the title Feat/frontmatter description fix: frontmatter description duplication (#194) Feb 5, 2021
@kiaking
Copy link
Member

kiaking commented Feb 9, 2021

Great fix! Thanks a lot 👍

@kiaking kiaking merged commit 338e845 into vuejs:master Feb 9, 2021
Mando75 added a commit to Mando75/vitepress that referenced this pull request Nov 20, 2021
Update UserConfig to accept an optional outDir and resolve it the same way vitepress resolves srcDir. The original .vitepress/dist behavior is provided as fallback if no user-provided outDir is present. For reference, see vuejs#170.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants