Allow filters to read non-text include files

[brewingcode]

I have some custom filters that I use with Pug's include, but the filters were choking because the incoming "text" from the include was wrong: it was automatically encoded as utf8 but my filters need Buffer.

So the simplest thing was to just expose a new option that, given the arguments to load.read, would simply tell Pug to either continue specifying utf8 encoding, or just leave the encoding out and return the Buffer.

A very stripped-down example looks like this:

// options.js
exports.skipEncoding = function(filename, options) { return filename.match(/png$/) };
exports.filters = {
  png: function(data, options) { /* do something heinous */ },
};

// my-template.pug
include:png image.png

I also noticed one broken test (unrelated to my changes) and fixed it, too.

This use-case is minor enough that I didn't even want to mention it in any docs.

[brewingcode]

I rebased out 1ccbcfa: it was a fix to what I thought were broken tests, but I guess my local setup is different from CI.

[ForbesLindesay]

I think a cleaner approach to this would be to do it as part of the filter. i.e. we could allow you to pass in a filter that looks like:

// options.js
exports.filters = {
  png: {renderBuffer: function(data, options) { /* do something heinous */ }},
};

We could check to see if a custom filter of that shape was passed when we're including files.

[rollingversions]

pug (2.0.4 → 3.0.0)

Breaking Changes

• read plugins must now return Buffer if you want to support filters that use renderBuffer

  If you don't wish to support this advanced use case, you can continue returning string. If you did not provide a read plugin, you do not need to do anything.

New Features

• Support filters that apply to Buffers

  e.g.

  // options.js
  exports.filters = {
    png: {
      // instead of a function, specify an object with a "renderBuffer" property
      // whose value is a function that takes a Buffer instead of a string
      renderBuffer: function(buffer, options) {
        var data = Buffer.from(buffer).toString('base64');
        return '<img src="data:image/png;base64, ' + data + '"/>';
      }
    }
  };

  You can then use the filter like:

  // foo.pug
  include:png my-small-image.png

pug-filters (3.1.1 → 3.2.0)

New Features

• Support filters that expect a Buffer instead of a string

  e.g.

  // options.js
  exports.filters = {
    png: {
      // instead of a function, specify an object with a "renderBuffer" property
      // whose value is a function that takes a Buffer instead of a string
      renderBuffer: function(buffer, options) {
        var data = Buffer.from(buffer).toString('base64');
        return '<img src="data:image/png;base64, ' + data + '"/>';
      }
    }
  };

  You can then use the filter like:

  // foo.pug
  include:png my-small-image.png

pug-load (2.0.12 → 3.0.0)

Breaking Changes

• read plugins must now return Buffer if you want to support filters that use renderBuffer

New Features

• File nodes now get a raw property that is a Buffer, in addition to the str

Packages With No Changes

The following packages have no user facing changes, so won't be released:

• pug-attrs
• pug-code-gen
• pug-error
• pug-lexer
• pug-linker
• pug-monorepo
• pug-parser
• pug-runtime
• pug-strip-comments
• pug-walk

Edit changelogs

[brewingcode]

Thanks for the reply! I was going for the absolute smallest change to accomplish what I needed.

a cleaner approach to this would be to do it as part of the filter

Okay, sounds reasonable. In order to add a magic key like renderBuffer to a filter, I'm looking here:

pug/packages/pug-filters/lib/handle-filters.js

Line 58 in bb0731f

function filterFileWithFallback(filter, filename, text, attrs) {

At this point, the file has already been read by pug-load, so are you suggesting that I re-read the file with fs in this function? I'm generally not a performance freak, but even I would think twice about reading the same file over and over (each use of the filter in my .pug template runs this filterFileWithFallback function, right?).

I also don't know all the other cases where a filter assumes its input is plain text, this filterFileWithFallback function is simply the first place pug barfed when I tried passing options: { filters: { png: { renderBuffer: ....

I guess what I'm asking is: could you give me a bit more guidance on how to safely add a renderBuffer check to the filtering module? I made my change at load.read() because it was the one single place where the files are read, AND only users who passed options.skipEncoding would ever be impacted by the change (so risk of breaking anybody else is zero). Changing a core part of pug was a bit more than I was willing to do.

[brewingcode]

Rebased against latest master (bb0731f), previously change was against 201b9c7

[ForbesLindesay]

I think the process should be:

1. change load.read to return a Buffer (https://github.com/pugjs/pug/blob/master/packages/pug-load/index.js#L83)
2. load.file can then just call .toString('utf8) on the buffer
3. If it's not a raw include with filters, we can call .toString('utf8') on https://github.com/pugjs/pug/blob/master/packages/pug-load/index.js#L28
4. in pug-filters, we can call .toString('utf8') before calling any filters that expect a string.

This will be a breaking change for pug-load, but should be non-breaking for everything else.

[brewingcode]

This will be a breaking change for pug-load, but should be non-breaking for everything else.

I mean, right now as a new option it's non-breaking to everyone. :P But yes, I do see the value in having this be a default, rather than some buried option.

I'll see if I can manage it by changing pug-load and pug-filters as per your steps.

[brewingcode]

OK, here's another attempt, slightly modified from your steps.

At first I tried your steps as-is, but step 3 (where I probably totally botched modifying https://github.com/pugjs/pug/blob/master/packages/pug-load/index.js#L28) completely exploded the tests with 55 failures, so I got spooked and went back to trying to change as few things as possible.

So I added a second property to node.file called node.file.raw (to live next to to node.file.str), and in pug-filters I use this new property if and only if the filter has the renderBuffer property. I do believe this does what I need in a non-breaking way, as well.

[trasherdk]

Please educate me.
Are you proposing to expose clients to binary data? Or are there something else going on here?

[brewingcode]

@trasherdk Yes, but only via a specially-crafted filter. No current pug users are going to start seeing Buffers getting barfed into HTML.

Here's a small example:

// options.js
exports.filters = {
  png: {
    // instead of a function, specify an object with a "renderBuffer" property
    // whose value is a function that takes a Buffer instead of a string
    renderBuffer: function(buffer, options) {
      var data = Buffer.from(buffer).toString('base64');
      return '<img src="data:image/png;base64, ' + data + '"/>';
    }
  }
};

// foo.pug
include:png my-small-image.png

[ForbesLindesay]

This looks good to me. @brewingcode can you just confirm that the change log (see the comment from Rolling Versions) looks correct, and then I'll merge this so it goes into the next release.

[brewingcode]

@ForbesLindesay thanks! I made the comment scan a little more naturally in the example options.js notes for the Rolling Version and hit "Save Changes". I also edited my comment above to match.

If there's anything else, lemme know!