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: duplicate meta tags #2164

Merged
merged 8 commits into from
Mar 23, 2020
Merged

fix: duplicate meta tags #2164

merged 8 commits into from
Mar 23, 2020

Conversation

haoranpb
Copy link
Contributor

@haoranpb haoranpb commented Feb 2, 2020

Summary

leave meta tags updating to updateMeta.js

fix #777 , relate to #2153

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Build-related changes
  • Other, please describe:

If changing the UI of default theme, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)

You have tested in the following browsers: (Providing a detailed version will be better.)

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature
  • Related documents have been updated
  • Related tests have been updated

To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

I'm not sure what to do when page meta is conflict with site meta, for example

// config.js
  head: [
    ['meta', { name: 'test-meta', content: 'this is a head meta' }]
  ]
---
# Markdown Front Matter
meta:
  - name: test-meta
    content: this is a page meta
---

merge them or use page meta to override site meta like description

@kefranabg
Copy link
Collaborator

Hi @ludanxer,

Thanks for the PR.

Here you're just removing the current page meta tags 😅

But what we would like is replacing the global meta tags with the current page meta tags if page meta tags exists. The solution would be a merge, with a priority on page meta tags.

@haoranpb
Copy link
Contributor Author

@kefranabg Thanks for your review and sorry for the delay.

I think I'm just removing one set of page meta tags. I understand how things look like, so I'll try to explain what I'm trying to do.

Head meta tags

Head meta tags are inserted differently in dev and build.

dev

tags: this.context.siteConfig.head || []

build

Page meta tags

Page meta tags are managed by updateMeta.js in both dev and build. However, another set of page meta tags are inserted by SSR.

dev

updateMeta.js

build

Both


and updateMeta.js which is causing the duplicate.

@kefranabg
Copy link
Collaborator

I just made a test with your branch, and global meta tag are always taking priority on page meta tag. It should be the opposite 😄

If global meta tags and page meta tag are defined, page meta tag have priority

@haoranpb
Copy link
Contributor Author

Need some time to fix that. Thanks!

@nuochong
Copy link
Contributor

nuochong commented Feb 21, 2020

I also found this problem. These problems (#112, #565, #665, #2133) two years ago seem to be finally fixed.
There is no problem in dev mode, it will appear after packaging. When the page template is packaged, the meta tag is written. The meta here is the meta tag of ssr. Due to the characteristics of the spa, the content in the head will not be refreshed, and after manually switching pages, a new meta tag will be dynamically added to the head. Causes meta tag confusion. Affects applications such as twitter, such as:

<meta name = "twitter: image" content = "a.png">
<meta name = "twitter: image" content = "b.png">

The above example will result in two pictures for twitter sharing
Solution:

  1. Overwrite when writing the meta dynamically. The meta permission of the current page is higher than the meta permission of the ssr loading page. (May increase the amount of calculation)
  2. Delete meta tag write in ssr (may not be good for seo)

@haoranpb
Copy link
Contributor Author

I made a test branch and deploy it. Hope it helps.

And I noticed

watch: {
  $page () {
     this.updateMeta()
  }
}

Keeps getting triggered when I scroll past headers in a page

@nuochong
Copy link
Contributor

Thank you for your efforts

@kefranabg
Copy link
Collaborator

Hey, I made some tests, if I add a the meta tag keywords on a page, I can't see it in the meta tag on the generated HTML page. Note that it will be added on runtime, but not on build time.

For example:

---
meta:
  - name: description
    content: hello
  - name: keywords
    content: super duper SEO
---
  • build the vuepress site
  • go to dist/path/to/my-page-with-meta
  • description will be generated in the html page but not keywords

Let me know if you want more context

@haoranpb haoranpb changed the title fix: duplicate meta in ssr fix: duplicate meta tags Feb 26, 2020
@haoranpb
Copy link
Contributor Author

I think it's ready now 🤔Thanks for the patience ❤️

@kefranabg
Copy link
Collaborator

@ludanxer Sorry for the delay, I'll do a review ASAP 😉

Copy link
Collaborator

@kefranabg kefranabg left a comment

Choose a reason for hiding this comment

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

I just added some suggestions and ask some questions. Please check everything is still working as expected if you apply changes 👍

packages/@vuepress/core/lib/node/build/index.js Outdated Show resolved Hide resolved
packages/@vuepress/core/lib/node/build/index.js Outdated Show resolved Hide resolved
Sun Haoran and others added 4 commits March 19, 2020 09:33
Previous method will init siteMetaTags with entry page meta tags instead of site meta tags
Copy link
Collaborator

@kefranabg kefranabg left a comment

Choose a reason for hiding this comment

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

LGTM, one more review and we'll get that merge 👍 thanks @ludanxer

@kefranabg kefranabg merged commit 01cd096 into vuejs:master Mar 23, 2020
return meta.map(m => {
let res = `<meta`
Object.keys(m).forEach(key => {
res += ` ${key}="${m[key]}"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be escaped? For example, this config.js:

module.exports = {
  description: 'Image cache & resize service',
}

renders as (within version 1.4.1):

    <meta name="description" content="Image cache & resize service">

which is probably not correct. fwiw, version 1.4.0 didn't had this problem.

Suggestion:

Suggested change
res += ` ${key}="${m[key]}"`
res += ` ${key}="${escape(m[key])}"`

and this import at the top of the file:

const escape = require('escape-html')

larionov pushed a commit to larionov/vuepress that referenced this pull request Aug 19, 2020
* fix: duplicate meta in ssr

* fix: page metas have higher priority

* Revert "fix: duplicate meta in ssr"

This reverts commit 5a02e2a.

* fix: render meta tags during ssr

* improve readability from suggestions

Co-Authored-By: Franck Abgrall <[email protected]>

* fix: missing spaces

* refactor: remove unnecessary code

* fix: siteMetaTags aren't correctly init

Previous method will init siteMetaTags with entry page meta tags instead of site meta tags

Co-authored-by: Franck Abgrall <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

meta data is doubled/tripled at runtime
5 participants