Skip to content

fix($core): Change meta merge function #2614 #2615

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

taichunmin
Copy link

@taichunmin taichunmin commented Sep 18, 2020

Summary

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:

close #2614

@taichunmin taichunmin changed the title fix meta type array #2614 fix #2614 change union rule to array type meta Sep 18, 2020
@taichunmin taichunmin changed the title fix #2614 change union rule to array type meta fix: Change union rule to array type meta #2614 Sep 18, 2020
@taichunmin taichunmin force-pushed the meta-array branch 2 times, most recently from 8d6e870 to 87f1bae Compare September 18, 2020 05:08
@taichunmin taichunmin changed the title fix: Change union rule to array type meta #2614 fix($core): Change union rule to array type meta #2614 Sep 18, 2020
@seifertm
Copy link
Contributor

seifertm commented Oct 7, 2020

Great work! I just ran into the same issue and was happy to see that someone already addressed it :)

I think that your PR only addresses a part of the problem, though. For example the og:image tag can have multiple values and is not included in META_TYPE_ARRAY. This opens a can of worms, because each og:image tag can have properties like og:image:width and og:image:height which would also need to be included.

I see two solutions for this:

  1. Make the list exhaustive. This is not a problem for the Open Graph specification, but Facebook, Twitter and the sorts use their own meta tags. Including all of them in the list and maintaining the list of tags might be a pain

  2. Instead of using a list of meta tags that can occur multiple times, find a smarter way than lodash.unionBy to merge the page and site meta tags. Do you have any ideas how we could do that?

@taichunmin
Copy link
Author

taichunmin commented Oct 7, 2020

I found another problem: the order of meta tags is very important.

For example:

<meta property="og:image" content="https://example.com/rock.jpg" />
<meta property="og:image:width" content="300" />
<meta property="og:image:height" content="300" />
<meta property="og:image" content="https://example.com/rock2.jpg" />
<meta property="og:image" content="https://example.com/rock3.jpg" />
<meta property="og:image:height" content="1000" />

means there are 3 images on this page, the first image is 300x300, the middle one has unspecified dimensions, and the last one is 1000px tall.


Because the meta property order is important, the following example should not be union:

<meta property="og:image" content="https://example.com/rock.jpg" />
<meta property="og:image:width" content="300" />
<meta property="og:image:height" content="300" />
<meta property="og:image" content="https://example.com/rock2.jpg" />
<meta property="og:image" content="https://example.com/rock3.jpg" />
<meta property="og:image:width" content="300" />
<meta property="og:image:height" content="300" />

Because it means there are 3 images on this page, the first image is 300x300, the middle one has unspecified dimensions, and the last one is 300x300.

@taichunmin
Copy link
Author

Maybe we should maintain a list for unique meta name? We can only deduplicate meta tags which name in the list, and we can expose the list to vueconfig?

@taichunmin
Copy link
Author

taichunmin commented Oct 7, 2020

Another method is merging siteMeta only if pageMeta didn't have any of same meta name?

@seifertm
Copy link
Contributor

seifertm commented Oct 8, 2020

Good point with the order of meta tags!

Another method is merging siteMeta only if pageMeta didn't have any of same meta name?

I think this would be the best solution. That way, we don't have to maintain or expose the META_TYPE array. It also means that users can provide default values in siteMeta and override them on a page-by-page basis in pageMeta. I really like the idea!

That's just my personal opinion, though. It would be nice to hear what the maintainers think :)

@taichunmin
Copy link
Author

@seifertm Okay, I will rewrite my PR to use this method.

@taichunmin taichunmin changed the title fix($core): Change union rule to array type meta #2614 fix($core): Change union rule to meta #2614 Oct 9, 2020
@taichunmin taichunmin changed the title fix($core): Change union rule to meta #2614 fix($core): Change meta merge function #2614 Oct 9, 2020
@taichunmin taichunmin force-pushed the meta-array branch 3 times, most recently from 738ae57 to d9c8a61 Compare October 9, 2020 06:35
@xr0master
Copy link
Contributor

Hey guys. Will it fix this issue?
#2607

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.

Can not have multiple meta "article:tag"
3 participants