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

Minify Templates based on a white list #14

Merged
merged 4 commits into from
Mar 14, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1,584 changes: 1,271 additions & 313 deletions __snapshots__/hbs-minifier-plugin.test.js.snap

Large diffs are not rendered by default.

7 changes: 7 additions & 0 deletions ember-cli-build.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@ const EmberAddon = require('ember-cli/lib/broccoli/ember-addon');
module.exports = function(defaults) {
let app = new EmberAddon(defaults, {
// Add options here
'ember-hbs-minifier': {
skip: {
elements: ['pre', 'address'],
classes: ['description'],
components: ['foo-bar']
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I'm not sure about the naming of whiteList, because the filters essentially blacklist us from doing our transforms. blackList sounds confusing too though. maybe just skip?

  • elementNodes -> elements

  • elementClassNames -> classes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skip sounds good. I have used elementClassNames to differentiate from component classNames since we cannot minify component boundaries(As classNames and classNameBindings defined in the js file will be resolved only at runtime).

}
});

/*
Expand Down
59 changes: 40 additions & 19 deletions hbs-minifier-plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,13 @@ const stripWhiteSpace = Util.stripWhiteSpace;
const isWhitespaceTextNode = Util.isWhitespaceTextNode;
const hasLeadingOrTrailingWhiteSpace = Util.hasLeadingOrTrailingWhiteSpace;
const stripNoMinifyBlocks = Util.stripNoMinifyBlocks;
const canTrimBlockStatementContent = Util.canTrimBlockStatementContent;
const canTrimElementNodeContent = Util.canTrimElementNodeContent;
const assignDefaultValues = Util.assignDefaultValues;

class HBSMinifierPlugin {

static createASTPlugin() {
class BasePlugin {
static createASTPlugin(config) {
config = config || {};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think at the point config should not be optional anymore.

let preStack = [];
let visitor = {
TextNode(node) {
Expand All @@ -21,13 +24,14 @@ class HBSMinifierPlugin {

BlockStatement: {
enter(node) {
if (node.path.original === 'no-minify') {
preStack.push(true);
let canTrim = canTrimBlockStatementContent(node, config);
if (!canTrim) {
preStack.push(node);
}
},

exit(node) {
if (node.path.original === 'no-minify') {
if (preStack[preStack.length - 1] === node) {
preStack.pop();
}
},
Expand All @@ -52,13 +56,19 @@ class HBSMinifierPlugin {

exit(node) {
node.body = stripNoMinifyBlocks(node.body);

if (preStack[preStack.length - 1] === node) {
preStack.pop();
}
},
},

ElementNode: {
enter(node) {
if (node.tag === 'pre') {
preStack.push(true);
let canTrim = canTrimElementNodeContent(node, config);

if (!canTrim) {
preStack.push(node);
}

if (preStack.length !== 0) {
Expand All @@ -79,7 +89,7 @@ class HBSMinifierPlugin {
exit(node) {
node.children = stripNoMinifyBlocks(node.children);

if (node.tag === 'pre') {
if (preStack[preStack.length - 1] === node) {
preStack.pop();
}
}
Expand All @@ -88,15 +98,26 @@ class HBSMinifierPlugin {

return { name: 'hbs-minifier-plugin', visitor };
}

transform(ast) {
let plugin = HBSMinifierPlugin.createASTPlugin();

this.syntax.traverse(ast, plugin.visitor);

return ast;
}

}

module.exports = HBSMinifierPlugin;
module.exports = function(config) {
config = config || {};
config = assignDefaultValues(config);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am missing the previous default config where (without any configuration) <pre> elements and the {{#no-minify}} "component" were skipped.

Copy link
Contributor Author

@siva-sundar siva-sundar Feb 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll check for no-minify and pre tag here and here respectively.


return class HBSMinifierPlugin extends BasePlugin {

transform(ast) {
let startLoc = ast.loc ? ast.loc.start : {};
/*
checking for line and column to avoid registering the plugin for ProgramNode inside a BlockStatement.
*/
if (startLoc.line !== 1 || startLoc.column !== 0) {
return ast;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this is necessary. Isn't the transform() function only called for top-level elements? The AST traversal should be handled by the traverse() call below.

Copy link
Contributor Author

@siva-sundar siva-sundar Feb 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In ember 2.15, the transform is called for all the Program Nodes. I am not sure why. Please check this pipeline and the changes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is a bug in Ember 2.15 that is already fixed again afaik

/cc @rwjblue

Copy link
Contributor Author

@siva-sundar siva-sundar Feb 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Turbo87 Do you want this issue to be patched up or should I keep it as an issue for that particular Ember versions?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should no guard against that. The transform that we do should be idempotent so the only advantage of this guard should be less work to do for the buggy versions.


let plugin = HBSMinifierPlugin.createASTPlugin(config);
this.syntax.traverse(ast, plugin.visitor);
return ast;
}
};
};
75 changes: 70 additions & 5 deletions hbs-minifier-plugin.test.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
'use strict';

/* eslint-env jest */

const defaultConfig = {
skip: {
elements: ['pre', 'address'],
classes: ['description'],
components: ['foo-bar']
}
};
const glimmer = require('@glimmer/syntax');
const HbsMinifierPlugin = require('./hbs-minifier-plugin');
const HbsMinifierPlugin = require('./hbs-minifier-plugin')(defaultConfig);

it('collapses whitespace into single space character', () => {
assert(`{{foo}} \n\n \n{{bar}}`);
Expand Down Expand Up @@ -45,17 +51,73 @@ it('does not collapse whitespace inside of {{#no-minify}} tags in other tags', f
assert(`<div>{{#no-minify}} \n\n \n{{/no-minify}}</div>`);
});

it('11. does not collapse multiple &nbsp; textNode into a single whitespace', function() {
it('does not collapse multiple &nbsp; textNode into a single whitespace', function() {
assert(`<span>1</span>&nbsp;&nbsp;<span>2</span>`);
});

it('12. does not collapse &nbsp; surrounding a text content into a single whitespace', function() {
it('does not collapse &nbsp; surrounding a text content into a single whitespace', function() {
assert(`<div>
<span> &nbsp;1&nbsp; </span>
<span> 2 </span>
</div>`);
});

it('does not minify `tagNames` specified in .hbs-minifyrc.js', function() {
assert(`<address>
Box 564,
<b>
Disneyland
</b>
<br>
<u> USA </u>
</address>`);
});

it('does not minify `classNames` specified in .hbs-minifyrc.js', function() {
assert(`<div class="description">
1
<span>
2
</span>
</div>`);
});

it('does not minify `components` specified in .hbs-minifyrc.js', function() {
assert(`{{#foo-bar}}
<span>
yield content
</span>
{{/foo-bar}}`);
});

Turbo87 marked this conversation as resolved.
Show resolved Hide resolved
it('minifies `tagNames` that are not specified in .hbs-minifyrc.js', function() {
assert(`<section>
Box 564,
<b>
Disneyland
</b>
<br>
<u> USA </u>
</section>`);
});

it('minifies `classNames` that are not specified in .hbs-minifyrc.js', function() {
assert(`<div class="contact-details">
John Smith
<i>
(Entrepreneur)
</i>
</div>`);
});

it('minifies `components` that are not specified in .hbs-minifyrc.js', function() {
assert(`{{#my-component}}
<span>
yield content
</span>
{{/my-component}}`);
});

function assert(template) {
let ast = process(template);
expect(ast).toMatchSnapshot();
Expand All @@ -65,9 +127,12 @@ function assert(template) {
}

function process(template) {
let plugin = () => {
return HbsMinifierPlugin.createASTPlugin(defaultConfig.skip);
};
return glimmer.preprocess(template, {
plugins: {
ast: [HbsMinifierPlugin.createASTPlugin]
ast: [plugin]
}
});
}
30 changes: 21 additions & 9 deletions index.js
Original file line number Diff line number Diff line change
@@ -1,18 +1,30 @@
/* eslint-env node */
'use strict';
let objectHash = require('object-hash');

module.exports = {
name: 'ember-hbs-minifier',

setupPreprocessorRegistry(type, registry) {
if (type === 'parent') {
let HbsMinifierPlugin = require('./hbs-minifier-plugin');
_setupPreprocessorRegistry(app) {
let registry = app.registry;
let options = app.options || {};
let config = options['ember-hbs-minifier'] || {};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the point at which we should apply the config defaults. If config.skip is not set we set it to { elements: ['pre'], components: ['no-minify'] } by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, if the user provides a config, he need to pass the defaults (pre and no-minify) along with his config like below. Is it ok?

{
  skip: {
    elements: ['pre', 'div'],
    components: ['no-minify', 'my-component']
  }
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I think that makes sense. otherwise it wouldn't be possible to not skip <pre>.


registry.add('htmlbars-ast-plugin', {
name: 'hbs-minifier-plugin',
plugin: HbsMinifierPlugin,
baseDir() { return __dirname; }
});
}
let HbsMinifierPlugin = require('./hbs-minifier-plugin')(config.skip || {});
registry.add('htmlbars-ast-plugin', {
name: 'hbs-minifier-plugin',
plugin: HbsMinifierPlugin,
baseDir() { return __dirname; },
cacheKey() { return `ember-hbs-minifier-${objectHash.MD5(config)}`; }
});
},

included(app) {
this._super.included.apply(this, arguments);
/*
Calling setupPreprocessorRegistry in included hook since app.options is not accessible
Refer PR: https://github.com/ember-cli/ember-cli/pull/7059
*/
this._setupPreprocessorRegistry(app);
}
};
3 changes: 3 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@
"jest": "^21.2.0",
"loader.js": "^4.2.3"
},
"dependencies": {
"object-hash": "1.2.0"
},
"engines": {
"node": ">= 4"
},
Expand Down
6 changes: 6 additions & 0 deletions tests/dummy/app/components/foo-bar.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import Ember from 'ember';
import layout from '../templates/components/foo-bar';

export default Ember.Component.extend({
layout
});
6 changes: 6 additions & 0 deletions tests/dummy/app/components/x-button.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import Ember from 'ember';
import layout from '../templates/components/x-button';

export default Ember.Component.extend({
layout
});
8 changes: 8 additions & 0 deletions tests/dummy/app/templates/components/foo-bar.hbs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
1
<span>
2
</span>
<span>
3
</span>
{{yield}}
2 changes: 2 additions & 0 deletions tests/dummy/app/templates/components/x-button.hbs
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
save
{{yield}}
Loading