-
Notifications
You must be signed in to change notification settings - Fork 9
Sitemap.xml is generated dynamically #81
base: master
Are you sure you want to change the base?
Conversation
This pull request introduces 7 alerts and fixes 2 when merging 19679e1 into 8acefcf - view on LGTM.com new alerts:
fixed alerts:
Comment posted by LGTM.com |
This pull request introduces 7 alerts and fixes 2 when merging 50b6176 into 8acefcf - view on LGTM.com new alerts:
fixed alerts:
Comment posted by LGTM.com |
This pull request introduces 7 alerts and fixes 2 when merging 5d66bf1 into 8acefcf - view on LGTM.com new alerts:
fixed alerts:
Comment posted by LGTM.com |
This pull request introduces 7 alerts and fixes 2 when merging 119460b into 8acefcf - view on LGTM.com new alerts:
fixed alerts:
Comment posted by LGTM.com |
@@ -0,0 +1,23 @@ | |||
{ | |||
"name": "fancy-sitemap", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should include a main, that way it doesn't have to guess where the entry file is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"main": "./lib"
start, | ||
} | ||
|
||
module.exports = SitemapGenerator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are the tests at? 😂
}); | ||
|
||
// TODO: Move to it's own repository and deploy... | ||
const sitemapGenerator = require('../../fancy-sitemap/index') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are in a workspace and it is defined as a package. Let the workspaces handle resolution.
@@ -7,6 +7,15 @@ const helmet = require('helmet') | |||
const session = require('express-session') | |||
const express = require('express') | |||
const compression = require('compression') | |||
const rateLimit = require('express-rate-limit'); | |||
|
|||
const crawlersRateLimit = rateLimit({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a real concern?
app.get('/sitemap.xml', (req, res) => { | ||
res.sendFile(path.join(__dirname + '/../sitemap.xml')) | ||
|
||
app.get('/sitemap.xml', crawlersRateLimit, (req, res) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible instead to generate this at build time and deploy it as part of the bundle? Then you don't need to continuously regenerate it. The reason to not do this if there is a way to dynamically change/add urls. If that is the case another option could be to create a webhook that generates the file as a result of an action(create post, delete post)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what you have is great and will work, but may be more performant to run it less.
<?xml version="1.0" encoding="UTF-8"?><urlset xmlns="http://www.sitemaps.org/schemas/sitemap/0.9" xmlns:news="http://www.google.com/schemas/sitemap-news/0.9" xmlns:xhtml="http://www.w3.org/1999/xhtml" xmlns:mobile="http://www.google.com/schemas/sitemap-mobile/1.0" xmlns:image="http://www.google.com/schemas/sitemap-image/1.1" xmlns:video="http://www.google.com/schemas/sitemap-video/1.1"><url><loc>https://www.ferreiro.me/</loc><changefreq>monthly</changefreq><priority>1.0</priority></url><url><loc>https://www.ferreiro.me/about</loc><changefreq>monthly</changefreq><priority>1.0</priority></url><url><loc>https://www.ferreiro.me/blog</loc><changefreq>monthly</changefreq><priority>1.0</priority></url><url><loc>https://www.ferreiro.me/talks</loc><changefreq>monthly</changefreq><priority>1.0</priority></url><url><loc>https://www.ferreiro.me/portfolio</loc><changefreq>yearly</changefreq><priority>0.8</priority></url><url><loc>https://www.ferreiro.me/contact</loc><changefreq>yearly</changefreq><priority>0.8</priority></url><url><loc>https://www.ferreiro.me/blog/interview-with-cesar-puerta-staff-software</loc><changefreq>monthly</changefreq><priority>0.9</priority></url><url><loc>https://www.ferreiro.me/blog/crushing-on-site-interviews</loc><changefreq>monthly</changefreq><priority>0.9</priority></url><url><loc>https://www.ferreiro.me/blog/jorge-ferreiro-joins-eventbrite-as-software-engineer</loc><changefreq>monthly</changefreq><priority>0.9</priority></url><url><loc>https://www.ferreiro.me/newsletter</loc><changefreq>yearly</changefreq><priority>0.8</priority></url><url><loc>https://www.ferreiro.me/blog/speed-up-your-website-frontend-8-practical-tips</loc><changefreq>monthly</changefreq><priority>0.9</priority></url><url><loc>https://www.ferreiro.me/resume/jorge_ferreiro_resume.pdf</loc><changefreq>yearly</changefreq><priority>0.8</priority></url><url><loc>https://www.ferreiro.me/blog/welcome-to-my-new-digital-space-welcome-to-jorge-ferreiro-blog</loc><changefreq>monthly</changefreq><priority>0.9</priority></url><url><loc>https://www.ferreiro.me/contact/talk</loc><changefreq>yearly</changefreq><priority>0.8</priority></url><url><loc>https://www.ferreiro.me/blog/category/software</loc><changefreq>monthly</changefreq><priority>0.9</priority></url><url><loc>https://www.ferreiro.me/contact/feedback</loc><changefreq>yearly</changefreq><priority>0.8</priority></url><url><loc>https://www.ferreiro.me/blog/category/product</loc><changefreq>monthly</changefreq><priority>0.9</priority></url><url><loc>https://www.ferreiro.me/blog/category/entrepreneurship</loc><changefreq>monthly</changefreq><priority>0.9</priority></url></urlset> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you are generating this should it be ignored not in git?
|
||
if (path) { | ||
// NB: If it returns an error, we don't care. | ||
await createSitemapFile(path, generatedXmlSitemap); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: createCacheFile() without waiting for it
) | ||
|
||
const start = (options) => ( | ||
// TODO: Check validity of the options object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Use Joi or other JSON schema validation to check it. https://github.com/hapijs/joi
This pull request introduces 7 alerts and fixes 2 when merging 764ca38 into 8653c8e - view on LGTM.com new alerts:
fixed alerts:
Comment posted by LGTM.com |
First version of this new package.
The idea is merge this with the project, and once it's ready, extract to it's own NPM package. (I'll go ahead now and reserve the name fancy-sitemap)