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

feat - #175 adds processSource option to the loader config to run han… #177

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@pizatski
Copy link

pizatski commented Jan 9, 2019

…dlebars template through html minifier

feat - #175 adds processSource option to the loader config to run han…
…dlebars template through html minifier
@@ -556,4 +556,20 @@ describe('handlebars-loader', function () {
});
});

});
/*

This comment has been minimized.

@houndci-bot

houndci-bot Jan 9, 2019

Expected indentation of 2 spaces but found 0 indent

@pizatski

This comment has been minimized.

Copy link

pizatski commented Jan 9, 2019

Looks like I auto-formatted a bunch of stuff. Let me know if that will be an issue. It passed the linter configured.

Also, I'm addressing the Hound violation.

@@ -6,6 +6,7 @@ var path = require("path");
var assign = require("object-assign");
var fastreplace = require('./lib/fastreplace');
var findNestedRequires = require('./lib/findNestedRequires');
var minify = require('html-minifier').minify;

This comment has been minimized.

@pizatski

pizatski Jan 9, 2019

Yes, this is an added dependency. I feel like it's probably better to add a known and tested library to do this sort of thing than writing my own regular expressions which will surely be buggy. Kudos to my colleague Clinton Bloodworth for the suggestion!

collapseWhitespace: true,
conservativeCollapse: true,
removeComments: true,
removeAttributeQuotes: true

This comment has been minimized.

@pizatski

pizatski Jan 9, 2019

These are fairly conservative options. More can be found here:
https://www.npmjs.com/package/html-minifier

@@ -202,14 +217,17 @@ module.exports = function(source) {

try {
if (source) {
if (query.processSource) {

This comment has been minimized.

@pizatski

pizatski Jan 9, 2019

I got a lint error when I double banged this. Let me know if that's okay.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 9, 2019

Coverage Status

Coverage increased (+0.2%) to 89.37% when pulling ff0172a on pizatski:175-add-html-minifier into 8cfcb2c on pcardune:master.

@pcardune
Copy link
Owner

pcardune left a comment

I'm not convinced that minification should be handled by this library. It's been a little while since I've been deep in webpack internals, but I seem to recall that there already existed some other libraries for doing html minification as part of a webpack pipeline.

Have you looked into that possibility at all?

Beyond this philosophical question, it woulds be nice to get rid of the unnecessary reformatting. I'm all for having one consistent format across all the code, but would prefer that any reformatting happen in its own PR with accompanying lint rules.

@@ -5,6 +5,11 @@
### Fixed
- Your improvement here...

## [1.7.2] - 2019-01-09

### Fixed

This comment has been minimized.

@pcardune

pcardune Jan 10, 2019

Owner
Suggested change Beta
### Fixed
### New Features
@@ -64,6 +64,7 @@ The following query (or config) options are supported:
- *ignorePartials*: Prevents partial references from being fetched and bundled. Useful for manually loading partials at runtime.
- *ignoreHelpers*: Prevents helper references from being fetched and bundled. Useful for manually loading helpers at runtime.
- *precompileOptions*: Options passed to handlebars precompile. See the Handlebars.js [documentation](http://handlebarsjs.com/reference.html#base-precompile) for more information.
- *processSource*: Runs the handlebars templates through an html minifier to remove whitespace and reduce the size of the compiled templates.

This comment has been minimized.

@pcardune

pcardune Jan 10, 2019

Owner

processSource is a very generic name that makes it hard to guess how exactly the source is being processed. Maybe something like minifyOutput would be better?

This comment has been minimized.

@pizatski

pizatski Jan 11, 2019

I think minifyOutput is a much clearer name. Will make the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment