Refactor

[ghost]

A complete refactor of the plugin, originated in #11. The plugin now uses sitemap.js, which means it no longer requires any maintenance of the actual sitemap generation logic.

I've also streamlined the API, so that for most people this:

.use(sitemap('http://www.mywebsite.com/'))

will suffice. I've also added an omitExtension and omitIndex option for those who rewrite their urls or use metalsmith-permalinks. Furthermore, all functionality is now covered by tests. One minor issue is that, for now, the hostname must have a trailing slash, but I hope that'll be resolved soon: ekalinin/sitemap.js#39.

See the readme for further explanation, and feel free to ask if there are any questions!

[ghost]

Also, if you've enable travis for this repo, let me know. I'd be happy to add a status badge to the readme!

[boushley]

@ismay this looks great. It's a pretty big change though, so I'll take a closer look on Monday.

[ghost]

It's a pretty big change though

Yeah definitely. Take your time, and let me know if there are any questions.

[ghost]

One minor issue is that, for now, the hostname must have a trailing slash, but I hope that'll be resolved soon: ekalinin/sitemap.js#39.

Now resolved.

[boushley]

@ismay this looks great!

I have a couple requests. First is around the way you're determining URL. You're defaulting to metadata.canonical. I think that's a reasonable default, but we should allow people to set what property they want to pull that value out of. In our main site for instance it is metadata.seo.canonical. You can use the lodash get method for this once you take an option. Similar request for last modified date. Previously these two options were urlProperty and modifiedProperty

I'm interested in why you would use make for running the test task instead of something like a test script inside of package.json.

[ghost]

Thanks!

I'm interested in why you would use make for running the test task instead of something like a test script inside of package.json.

Just personal preference. I like the simplicity of working with make, but you can use npm as well.

We should allow people to set what property they want to pull that value out of. [...] Similar request for last modified date.

I saw that, but it was a deliberate decision to hardcode these. It might be a good idea to scope them inside of a sitemap option, to prevent conflicts, but I don't see any added value in making the values configurable tbh. It'll only add unnecessary complexity. What is the added value in making them configurable?

[boushley]

The added value is that it works for more people.

As it stands my company would have to change other plugins to write these values at the specified locations. I'm fine with these as defaults, but we should make it configurable. If you'd rather not I'll go ahead and land this and then add that functionality.

[ghost]

Feel free to modify it. I'd prefer to keep it as simple as possible, but it's your plugin. 👍