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: closes #69 and & adds sitemap: false option to pages #70

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

SnirShechter
Copy link
Contributor

@SnirShechter SnirShechter commented Jun 16, 2019

Closes #69 - adds data to routes in the filter function.

Also closes #c55 (for some reason the issue was not posted in github).

@codecov
Copy link

codecov bot commented Jun 16, 2019

Codecov Report

Merging #70 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff         @@
##            dev    #70   +/-   ##
===================================
  Coverage   100%   100%           
===================================
  Files         1      1           
  Lines         3      3           
===================================
  Hits          3      3

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 a7a3bca...a6d2f83. Read the comment docs.

@SnirShechter
Copy link
Contributor Author

@NicoPennec
Can you take a look, please?

@codecov-io
Copy link

Codecov Report

Merging #70 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff         @@
##            dev    #70   +/-   ##
===================================
  Coverage   100%   100%           
===================================
  Files         1      1           
  Lines         3      3           
===================================
  Hits          3      3

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 a7a3bca...a6d2f83. Read the comment docs.

1 similar comment
@codecov-io
Copy link

Codecov Report

Merging #70 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff         @@
##            dev    #70   +/-   ##
===================================
  Coverage   100%   100%           
===================================
  Files         1      1           
  Lines         3      3           
===================================
  Hits          3      3

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 a7a3bca...a6d2f83. Read the comment docs.

@SnirShechter SnirShechter changed the title feat: closes #69 - adds data to routes in the filter function feat: closes #69 and #70 Jun 20, 2019
@SnirShechter SnirShechter changed the title feat: closes #69 and #70 feat: closes #69 and & adds sitemap: false option to pages Jun 20, 2019
@oriati
Copy link

oriati commented Jun 20, 2019

this looks great!

@SnirShechter
Copy link
Contributor Author

@pi0 @NicoPennec
Guys, is this module even maintained?

@NicoPennec
Copy link
Member

Thanks @SnirShechter for your proposal 🙏
It's a great idea in order to enhance the module configuration.

Sorry for the delay, I will test and review your PR this week!

@NicoPennec NicoPennec self-requested a review September 4, 2019 21:55
Copy link
Member

@NicoPennec NicoPennec left a comment

Choose a reason for hiding this comment

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

Can you make a benchmark before / after the addition of your feature?

I want to validate the right performance with the "acorn" parser during the generation of sitemap.

@@ -232,7 +236,7 @@ function flattenRoutes (router, path = '', routes = []) {
flattenRoutes(r.children, path + r.path + '/', routes)
}
if (r.path !== '') {
routes.push(path + r.path)
routes.push({ ...r, url: path + r.path })
Copy link
Member

Choose a reason for hiding this comment

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

This update can create a breaking change in the usage of the filter option of the sitemap module, no ?

if yes, your commit message must respect the conventional commit to bump the release version as "major" update by the release script

@@ -0,0 +1,4 @@
module.exports = {
COMPONENT_OPTIONS_BLOCK: 'script',
Copy link
Member

Choose a reason for hiding this comment

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

In any <page>.vue file, it will always be a script element.

Why create a constant?

@@ -0,0 +1,4 @@
module.exports = {
COMPONENT_OPTIONS_BLOCK: 'script',
COMPONENT_OPTIONS_KEY: 'sitemap'
Copy link
Member

Choose a reason for hiding this comment

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

To be customisable, you can set that key in the sitemap configuration in nuxt.config.js instead of create a constant.

const compiler = require('vue-template-compiler')
// const { COMPONENT_OPTIONS_BLOCK, COMPONENT_OPTIONS_KEY } = require('constants')

function extractComponentOptions (path, blockName, key) {
Copy link
Member

Choose a reason for hiding this comment

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

I think you should rename the function and file from extractComponentOptions to extractPageOptions, because it is relative to the *.vue file in the Pages folder.

</template>
<script>
export default {
sitemap:false
Copy link
Member

Choose a reason for hiding this comment

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

Why limit the sitemap option to a simple boolean for exclude / include a page from the sitemap?

Maybe you can support the following options in addition of sitemap: true :

<script>
  export default {
    sitemap: {
      changefreq: 'daily',
      priority: 1,
      lastmod: new Date(),
      // ...
   }

@SnirShechter
Copy link
Contributor Author

@NicoPennec
Thanks for the feedback. I'll get right on it ASAP.

@yurks
Copy link

yurks commented Feb 12, 2020

just similar implementation with no extra deps #101

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.

Filter function should receive the full route object (instead of just the url)
5 participants