11
\$\begingroup\$

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;
    });
})
\$\endgroup\$
  • \$\begingroup\$ NOTE If you want to suggest a better solution: Please go to the StackOverflow question and post an answer. \$\endgroup\$ – James Khoury Sep 23 '14 at 2:01
  • \$\begingroup\$ From a code review perspective this looks totally fine to me. Except that you should comment why you are doing it ( Visual Studio adds a BOM ) Also comment why you would want to call done before you are done. \$\endgroup\$ – konijn Sep 23 '14 at 13:59
  • \$\begingroup\$ @konijn I am not sure about the done call ... i copied it from somewhere else. I am using visual studio because I like the environment. I dont seem to have any control over how it encodes. \$\endgroup\$ – James Khoury Sep 23 '14 at 21:34
4
+50
\$\begingroup\$

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:

  1. Stripping the BOM, of course, which is indeed what the function claims to do
  2. 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)
})
\$\endgroup\$
  • \$\begingroup\$ I was starting to think about better structuring the function like other "real" plugins. for example github.com/aymericbeaumet/metalsmith-concat/blob/master/lib/… \$\endgroup\$ – James Khoury Oct 23 '14 at 23:50
  • \$\begingroup\$ @JamesKhoury Not a bad idea. And you're pretty close already. A lot of the example you linked is option parsing and module boilerplate to allow it to be required. The "meat" isn't really any different from what you have. \$\endgroup\$ – Flambino Oct 24 '14 at 0:12
  • \$\begingroup\$ I think I will be following your single plugin solution with some of that boilerplate code. \$\endgroup\$ – James Khoury Oct 28 '14 at 1:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.