Metalsmith plugin for stripping UTF8 BOM from files I've been developing a metalsmith static site and I came across an issue where Visual Studio was automatically adding a BOM to the pages. 

I wrote the following plugin for metalsmith (it needs to be run as the first plugin). This feels a little messy. It was written mostly as a copy-paste and a lot of debugging.

var stripBom = require('strip-bom');
var front = require('front-matter');
var extend = require('extend');

// **snip**

.use(function __utf8BOM_workaround(files, metalsmith, done)
{
    setImmediate(done);
    Object.keys(files).forEach(function (file)
    {
        var data = files[file];
        var parsed = front(stripBom(data.contents.toString()));
        data = extend({}, data, parsed.attributes);
        data.contents = new Buffer(parsed.body);

        files[file] = data;
    });
})
As @konijn said in the comments, it looks pretty OK, though I do have some comments. I don't know anything about Metalsmith specifically, so it'll just be more general JS/Node comments.

Firstly, it looks like you're doing two different things here:


Stripping the BOM, of course, which is indeed what the function claims to do
Parsing some the file, and extracting some attributes.


I don't know the exact structure of the input for this function, but each "file" object seems to contain a contents buffer. Question is whether it also contains other attributes already, or if they only get added by this function.

If it's the latter, then the plugin is really two things, which doesn't smell quite right. I'd probably break it into two plugins: Strip the BOM somewhere, then parse the attributes somewhere else. I imagine you need to strip the BOM before anything else, but beyond that, those two operations sound independent to me.

I can see the appeal of keeping it in the same place though, since splitting it up requires and extra toString and buffer replacement per file. But the overhead is, I'd wager, completely negligible. 

The alternative would be to call the plugin something like utf8SafeParsing (or something like that), just to indicate that its main function is to parse - compared to that, the BOM-stripping seems like a minor thing.

Besides that I can only think minor improvements.


Firstly, there are konijn's two observations: A comment about why this plugin is used (why the BOM is a problem, and/or where it comes from), and the call to setImmediate(done) before actually being done. The latter probably works just fine as there's nothing asynchronous going on in the function, but nevertheless seems odd to call it before doing anything.
I always lobby for using brace-on-same-line style in JavaScript, only because putting the brace after a linebreak can in some (uncommon) circumstances cause JS auto-insert a semicolon at the linebreak, causing all sorts of confusion. In other words, while whitespace shouldn't matter, it kinda sorta does, and the interpreter kinda sorta assumes that you're using brace-on-same-line, and will attempt to "fix" your code otherwise.
You can move your require calls into the function itself, to keep things neat and tidy (requires are cached, so there's no performance hit).
If you keep it as one function, you could add a little helper function or two for yourself. No difference in logic, just structure.
Super trivial thing; I'd name the function stripBOM rather than mix-case the acronym. If the named started with "BOM", then it should be lowercase, but acronyms in JS names are typically kept uppercase (e.g. innerHTML in browsers). But again, this nit-picking in extenso.


Again, though, the code seems quite fine as-is. It's readable, and if you say it works, then great.



Here are some refactoring attempts with really, really minor changes. 

2 plugin solution:

// **snip**

.use(function stripUTF8BOM(files, metalsmith, done) {
    var stripBOM = require('strip-bom');

    Object.keys(files).forEach(function (file) {
        var data = files[file],
            parsed = stripBOM(data.contents.toString());

        files[file].contents = new Buffer(parsed.body);
    });

    setImmediate(done);  // or, presumably, just call `done();` directly
})
.use(function parseAttributes(files, metalsmith, done) {
    var front = require('front-matter'),
        extend = require('extend');

    Object.keys(files).forEach(function (file) {
        var data = files[file],
            parsed = front(data.contents.toString());

        files[file] = extend({}, parsed.attributes, {
          contents = new Buffer(parsed.body);
        });
    });

    setImmediate(done); // or, presumably, just call `done();` directly
})


Single plugin solution:

// **snip**

.use(function utf8SafeParsing(files, metalsmith, done) {
    var stripBOM = require('strip-bom'),
        front = require('front-matter'),
        extend = require('extend');

    function parse(buffer) {
      var sanitized = stripBOM(buffer.toString());
      return front(sanitized);
    }

    Object.keys(files).forEach(function (file) {
        var parsed = parse(files[file].contents);

        files[file] = extend({}, parsed.attributes, {
          contents = new Buffer(parsed.body);
        });
    });

    setImmediate(done);  // (see above)
})