Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow filters to read non-text include files #3213

Merged
merged 10 commits into from May 19, 2020
Merged

Conversation

@brewingcode
Copy link
Contributor

@brewingcode brewingcode commented Feb 8, 2020

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 brewingcode force-pushed the brewingcode:master branch from 67f2051 to 7453ed5 Feb 8, 2020
@brewingcode
Copy link
Contributor Author

@brewingcode brewingcode commented Feb 8, 2020

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.

Copy link
Member

@ForbesLindesay ForbesLindesay left a comment

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
Copy link

@rollingversions rollingversions bot commented May 1, 2020

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
Copy link
Contributor Author

@brewingcode brewingcode commented May 2, 2020

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:

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.

This allows Pug to correctly read non-utf8 files, if the user wants to
do such a thing.

The optional function takes the same arguments as load.read(), and
should return a truthy/falsey value. If truthy, load.read() will NOT
specify an encoding when it reads the file from disk. Otherwise, it will
continue to specify "utf-8" as the encoding.
@brewingcode brewingcode force-pushed the brewingcode:master branch from 7453ed5 to 2062747 May 2, 2020
@brewingcode
Copy link
Contributor Author

@brewingcode brewingcode commented May 2, 2020

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

@ForbesLindesay
Copy link
Member

@ForbesLindesay ForbesLindesay commented May 3, 2020

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
Copy link
Contributor Author

@brewingcode brewingcode commented May 4, 2020

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
Copy link
Contributor Author

@brewingcode brewingcode commented May 6, 2020

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.

@brewingcode brewingcode changed the title Add options.skipEncoding Allow filters to read non-text include files May 6, 2020
@trasherdk
Copy link

@trasherdk trasherdk commented May 6, 2020

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

brewingcode added 2 commits May 6, 2020
These render as objects with `type: "Buffer"`, along with the `size` and
`hash` of the data in the buffer.
@brewingcode
Copy link
Contributor Author

@brewingcode brewingcode commented May 6, 2020

@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
Copy link
Member

@ForbesLindesay ForbesLindesay left a comment

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
Copy link
Contributor Author

@brewingcode brewingcode commented May 19, 2020

@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!

@ForbesLindesay ForbesLindesay merged commit 9e96bb7 into pugjs:master May 19, 2020
4 checks passed
4 checks passed
@github-actions
test (10.x)
Details
@github-actions
test (12.x)
Details
@github-actions
test (14.x)
Details
@rollingversions
RollingVersions releasing multiple packages
Details
@ForbesLindesay ForbesLindesay mentioned this pull request May 24, 2020
This was referenced Mar 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants