Swaagie / minimize

Minimize HTML
MIT License
162 stars 18 forks source link

Handlebars helper pass through #53

Closed nddery closed 8 years ago

nddery commented 8 years ago

Related to #52, this PR adds a pass through, through the handlebars option, to not parse Handlebars helpers as attribute when they reside outside of an element attribute. When parsing an element attributes, it looks for the Handlebars template delimiter ({{) and if it finds it at the beginning of the attribute name, it returns unparsed attributes + key until it finds the closing delimiter (}}). If an "attributes" has a value, it parses it correctly.

What do you think ? Is this something that's too "hacky" ? I've tested with a bunch of different options enabled and with different attributes, before, after, in between and it seems to be pretty solid. My main concern is that I'm not familiar with the way htmlparser2 splits elements attributes into it's tree, so I may well be forgetting test cases.

Swaagie commented 8 years ago

Thanks for your hard work on this. I really dig the idea of minifying at any point (e.g. before or after template parsing). But I'm afraid I can't merge this. First, the feature itself does not cover all handlebars integrations as handlebar code could be between elements and basically anywhere within the HTML. I'm curious how htmlparser2 would parse the code in other cases, outside attributes that is.

Second I'd like to keep the core of minimize itself clean, e.g. it should just do HTML parsing. However I think this feature could work as plugin. And I'd like to suggest to move this logic to a plugin. Keeping this open for now to discuss the merits of this feature.

nddery commented 8 years ago

I'll see how I could move this to a plugin, however, in the README, it mentions to register plugins like so:

new Minimize({
  plugins: [
    element: function(node, next) { /* do something */ next(); }
  ]
});

However, that is not valid JS, I've tried replacing the plugins array by an object and while it did not throw any error this time, it also didn't run my code. How should I go about registering plugins ?

As for your concern about this not covering all handlebars integrations, we've been using minimize on our handlebars templates for quite some time without any issue. This seems to be the only case where the output get's tangled (not to say that there aren't others...). For example, the following piece of HTML

<div class="widgets">
  {{#forEach widgets}}
    <h2 id="{{this.id}}" class="widget">{{this.name}}</h2>
  {{/forEach}}
</div>

is parsed to the following htmlparser2 tree:

[ { type: 'tag',
    name: 'div',
    attribs: { class: 'widgets' },
    children: 
     [ { data: '   {{#forEach widgets}}     ',
         type: 'text',
         next: 
          { type: 'tag',
            name: 'h2',
            attribs: { id: '{{this.id}}', class: 'widget' },
            children: 
             [ { data: '{{this.name}}',
                 type: 'text',
                 next: null,
                 prev: null,
                 parent: [Circular] } ],
            next: 
             { data: '   {{/forEach}} ',
               type: 'text',
               next: null,
               prev: [Circular],
               parent: [Circular] },
            prev: [Circular],
            parent: [Circular] },
         prev: null,
         parent: [Circular] },
       { type: 'tag',
         name: 'h2',
         attribs: { id: '{{this.id}}', class: 'widget' },
         children: 
          [ { data: '{{this.name}}',
              type: 'text',
              next: null,
              prev: null,
              parent: [Circular] } ],
         next: 
          { data: '   {{/forEach}} ',
            type: 'text',
            next: null,
            prev: [Circular],
            parent: [Circular] },
         prev: 
          { data: '   {{#forEach widgets}}     ',
            type: 'text',
            next: [Circular],
            prev: null,
            parent: [Circular] },
         parent: [Circular] },
       { data: '   {{/forEach}} ',
         type: 'text',
         next: null,
         prev: 
          { type: 'tag',
            name: 'h2',
            attribs: { id: '{{this.id}}', class: 'widget' },
            children: 
             [ { data: '{{this.name}}',
                 type: 'text',
                 next: null,
                 prev: null,
                 parent: [Circular] } ],
            next: [Circular],
            prev: 
             { data: '   {{#forEach widgets}}     ',
               type: 'text',
               next: [Circular],
               prev: null,
               parent: [Circular] },
            parent: [Circular] },
         parent: [Circular] } ],
    next: null,
    prev: null,
    parent: null } ]

which in turn is rendered by minimize as follow:

<div class=widgets>{{#forEach widgets}}<h2 id={{this.id}} class=widget>{{this.name}}</h2>{{/forEach}}</div>
Swaagie commented 8 years ago

Updated the docs, basically a plugin is just a module that should export an id (string) and element (function). The element function receives element, next. Should you encounter more problems let me know.

nddery commented 8 years ago

Oh! Yes, that make sense, for some reason I thought that the id and element items were 2 different plugins... THanks!

nddery commented 8 years ago

My understanding of plugins is that they can modify the tree structure created by htmlparser2 before minimize works it's magic, right ?

I've created a plugin that rewrite the attribs property of elements in the htmlparser2 tree before passing it to minimize. It is available here. I just ran into problems where htmlparser2 removes slashes inside attributes. This will need much more thought before actually making it public.

Swaagie commented 8 years ago

Yeah I was afraid htmlparser2 would be limiting, the only way around this would be a custom parser instance accompanied by a plugin perhaps. Plugin might not be needed anymore even with a custom parser.

nddery commented 8 years ago

I've updated the plugin to handle stuff like "../../object.property". It feels a bit hacky though. However, it seems to be working now and we're using it during development. So far no issues and we're minimizing hundreds of templates files. I'll be reassessing the need of a custom parser (which I do not have the technical capabilities to write) when problems arise again. Feel free to close this PR (and the associated issue).

Swaagie commented 8 years ago

Will do in a few, I'll link the plugin from the docs so other people can benefit from your work :)

Swaagie commented 8 years ago

Closing this as the functionality is available as plugin, does the plugin cover all edge cases btw? If so i'd like to add it to the docs still.

nddery commented 8 years ago

I've added tests to my plugin, a lot of them taken directly from your test suite (but adding my plugin to each test to see how it would interact with currently working code). All tests passes as of now.

As I've mentioned above, we are using Minimize with the plugin in development and soon production and haven't witness any issues (apart from the fact that the plugin output is all lowercase'd) yet.

There is a known issue with the spare option, which I haven't figured out.

I can't say that the plugin covers all edge cases for sure but as I've said, it seems to be working quite well for us so far.