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: dedupe meta objects by VMID #282

Merged
merged 2 commits into from
Dec 18, 2018
Merged

feat: dedupe meta objects by VMID #282

merged 2 commits into from
Dec 18, 2018

Conversation

manniL
Copy link
Member

@manniL manniL commented Nov 6, 2018

related: nuxt/nuxt#4282

Whether or not that feature should be supported by vue-meta is another thing to discuss.

@atinux
Copy link
Member

atinux commented Nov 26, 2018

It should yes

@codecov
Copy link

codecov bot commented Dec 18, 2018

Codecov Report

Merging #282 into master will increase coverage by 0.44%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #282      +/-   ##
=========================================
+ Coverage   40.15%   40.6%   +0.44%     
=========================================
  Files          18      18              
  Lines         264     266       +2     
=========================================
+ Hits          106     108       +2     
  Misses        158     158
Impacted Files Coverage Δ
src/shared/getMetaInfo.js 94.64% <ø> (ø) ⬆️
src/shared/getComponentOption.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e95381f...c590d8a. Read the comment docs.

@manniL manniL changed the title test: failing for duplicate VMID in same metaInfo feat: dedupe meta objects by VMID Dec 18, 2018
@@ -64,6 +66,10 @@ export default function getComponentOption (opts, result = {}) {

return metaObject
})
result.meta = uniqBy(
result.meta.reverse(),
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 had to reverse the list to always consider the last meta object as the "true" one

@@ -64,6 +66,10 @@ export default function getComponentOption (opts, result = {}) {

return metaObject
})
result.meta = uniqBy(
result.meta.reverse(),
metaObject => metaObject.hasOwnProperty(tagIDKeyName) ? metaObject[tagIDKeyName] : uniqueId()
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 had to implement a workaround for tags without vmid set at all (which is common if you want to multiple tags of the same type)

My first idea was to use Symbol but it is not supported in IE :(

So I had to use another lodash fn

@atinux atinux merged commit 7d94107 into nuxt:master Dec 18, 2018
@manniL manniL deleted the test-multiple-attrs-same branch December 18, 2018 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants