-
Notifications
You must be signed in to change notification settings - Fork 17
Conversation
lib/generate-sitemap.js
Outdated
sitemapStatic( | ||
sitemapWriter, | ||
{ | ||
try { |
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 that try-catch is not going to work here, either. On closer inspection, I think we'll have to rely on sitemapWriter
emitting its stream events. We should add sitemapWriter.on('error', reject)
and sitemapWriter.on('finish', () => resolve())
, I think.
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.
That makes sense, done!
Is there a reason you used the anonymous function wrapper in your example? Just curious, I wrote it as follows, but I can change it!
sitemapWriter.on('error', reject);
sitemapWriter.on('finish', resolve);
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.
The only reason I had in mind was strict control over the value with which the Promise will resolve. With () => resolve()
we can say that this function returns a Promise that resolves with undefined
. With just resolve
, I'm not exactly sure what it will resolve with — would have to check the Node docs — and I'm not sure it'd be valuable to the consumer of this function.
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.
That makes sense! I updated it.
Interested in sending a PR to sitemap-static for fixing the docs in https://github.com/tmcw/sitemap-static#library-api? |
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.
Other than that stream-related trickery, looks great 👍
If a user doesn't specify
siteOrigin
, then Batfish will now do the following:prefixUrl.absolute
I also modified
lib/generate-sitemap.js
. I added a try/catch block to fix promise resolution, because it turns out thesitemap-static
package doesn't accept a callback (check out the function).Closes #43
@davidtheclark 👀