Minify Templates based on a white list #14

siva-sundar · 2017-09-29T10:19:28Z

Implements #9

index.js

Turbo87 · 2017-10-14T10:24:25Z

@siva-sundar I'm still seeing tests that have changed. I thought you adjusted the PR?

siva-sundar · 2017-10-15T06:51:02Z

Since we are collapsing the leading and trailing whitespaces into a single whitespace, the existing test cases needed to be updated. Let me know if there is anything else needs to be done.

Turbo87 · 2017-10-16T08:02:01Z

@siva-sundar that is a separate change though that seems unrelated to the intent of this PR which was adding support for whitelisting components. please note that the amount of changes in this PR make it very hard to review and it is usually much easier for maintainers to review smaller PRs that only change or add single features.

siva-sundar · 2017-10-17T11:20:31Z

I have moved the changes other than the whitelisting into a separate PR(#16). Once it is merged, I'll update this PR.

Turbo87

Don't have much time atm as I'm going on vacation today for a week. I'll have a detailed look once I'm back.

Turbo87 · 2017-10-28T08:47:36Z

hbs-minifier-plugin.js

+class BasePlugin {
+  constructor(env) {
+    env = env || {};
+    this.moduleName = env.moduleName;


this seems unused now

ember-cli-build.js

hbs-minifier-plugin.test.js

index.js

Turbo87 · 2018-02-05T18:49:19Z

@siva-sundar I've rebased your branch on top of all the changes that fixed our CI builds. It seems that some tests are now failing though... 😞

siva-sundar · 2018-02-07T17:52:02Z

@Turbo87 Tests are failing since htmlbars adds text node boundaries (at the begining and the end) of a template file.

Currently I am checking for two text nodes here. But for ember <= 2.9.1, there will be an additional textNode created to mark the closeBoundary.

Shall I update the test case based on a version check ?

Turbo87 · 2018-02-08T07:15:43Z

Shall I update the test case based on a version check ?

I've updated the existing tests to essentially .filter(el => el.textContent !== '') and then did the elements.length check afterwards. This was enough to make it pass on all 2.x versions of Ember without any explicit version checks.

siva-sundar · 2018-02-08T14:46:32Z

I have updated the test cases. Can you have a look at it?

Turbo87 · 2018-02-19T14:03:55Z

hbs-minifier-plugin.js

+      */
+      if (startLoc.line !== 1 || startLoc.column !== 0) {
+        return ast;
+      }


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.

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.

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

/cc @rwjblue

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

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.

Turbo87 · 2018-02-19T14:04:41Z

hbs-minifier-plugin.js

-module.exports = HBSMinifierPlugin;
+module.exports = function(config) {
+  config = config || {};
+  config = assignDefaultValues(config);


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

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

Turbo87 · 2018-02-19T14:07:02Z

utils/helpers.js

+}
+
+
+const canTrimBlockStatementContent = function(node, config) {


please convert those to named functions instead of const + anonymous function

Turbo87 · 2018-02-19T14:07:34Z

utils/helpers.js

+  if (config.components.indexOf(componentName) !== -1 || componentName === 'no-minify' || config.components === 'all') {
+    return false;
+  }
+  return true;


this can be simplified by returning the inverse of the existing condition

Turbo87 · 2018-02-19T14:08:14Z

utils/helpers.js

+
+const assignDefaultValues = function(config) {
+  config = config || {};
+  let elements = config.elements || 'all';


why are we skipping all by default?

siva-sundar · 2018-02-19T15:36:28Z

I have updated the PR with the changes mentioned. Please take a look again.

Turbo87 · 2018-02-21T08:00:31Z

hbs-minifier-plugin.js

-  static createASTPlugin() {
+class BasePlugin {
+  static createASTPlugin(config) {
+    config = config || {};


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

Turbo87 · 2018-02-21T08:03:46Z

index.js

+  _setupPreprocessorRegistry(app) {
+    let registry = app.registry;
+    let options = app.options || {};
+    let config = options['ember-hbs-minifier'] || {};


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.

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'] } }

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

Turbo87 · 2018-02-21T08:10:02Z

utils/helpers.js

+  });
+}
+
+function canTrimUnnecessaryWhiteSpace(value, config) {


the method name should in some way suggest that this is handling values of the class property

do we need to pass in the complete config here or would config.classes be enough?

siva-sundar force-pushed the patch_hbs_minify_config branch 20 times, most recently from 98c5fd7 to 53dde0b Compare October 4, 2017 05:58

siva-sundar changed the title ~~[WIP] Minify Templates based on a white list~~ Minify Templates based on a white list Oct 4, 2017

siva-sundar commented Oct 4, 2017

View reviewed changes

index.js Outdated Show resolved Hide resolved

siva-sundar force-pushed the patch_hbs_minify_config branch 2 times, most recently from 93b3586 to d49229f Compare October 4, 2017 14:08

siva-sundar changed the title ~~Minify Templates based on a white list~~ [WIP] Minify Templates based on a white list Oct 17, 2017

siva-sundar force-pushed the patch_hbs_minify_config branch from 44fba8f to 80c7479 Compare October 27, 2017 14:48

Turbo87 reviewed Oct 28, 2017

View reviewed changes

Turbo87 reviewed Nov 24, 2017

View reviewed changes

hbs-minifier-plugin.test.js Show resolved Hide resolved

index.js Outdated Show resolved Hide resolved

index.js Outdated Show resolved Hide resolved

siva-sundar force-pushed the patch_hbs_minify_config branch from 061fc80 to b23073a Compare December 18, 2017 04:17

Turbo87 reviewed Dec 18, 2017

View reviewed changes

index.js Outdated Show resolved Hide resolved

index.js Outdated Show resolved Hide resolved

siva-sundar force-pushed the patch_hbs_minify_config branch from d306f81 to d89c2e1 Compare January 2, 2018 11:26

Turbo87 force-pushed the patch_hbs_minify_config branch 2 times, most recently from e84a15b to ea4ee45 Compare February 5, 2018 18:08

siva-sundar force-pushed the patch_hbs_minify_config branch 5 times, most recently from 0e459ea to 1b517e2 Compare February 8, 2018 14:12

Provide a config to skip minification

d5ae5e5

siva-sundar force-pushed the patch_hbs_minify_config branch from 1b517e2 to d5ae5e5 Compare February 8, 2018 14:21

Turbo87 reviewed Feb 19, 2018

View reviewed changes

Minify all nodes by default

3962fb0

siva-sundar force-pushed the patch_hbs_minify_config branch from 025f43a to 3962fb0 Compare February 19, 2018 15:33

Turbo87 reviewed Feb 21, 2018

View reviewed changes

Assign default config

a4cd393

siva-sundar force-pushed the patch_hbs_minify_config branch from 188fd89 to acdbf90 Compare February 26, 2018 06:15

update README

acdbf90

cibernox approved these changes Mar 11, 2018

View reviewed changes

Turbo87 merged commit 46dd8d9 into mainmatter:master Mar 14, 2018

Turbo87 mentioned this pull request Mar 15, 2018

Discussion: Preserve white space based on a whitelist #9

Closed

Turbo87 added the enhancement label Apr 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minify Templates based on a white list #14

Minify Templates based on a white list #14

siva-sundar commented Sep 29, 2017 •

edited

Loading

Turbo87 commented Oct 14, 2017

siva-sundar commented Oct 15, 2017 •

edited

Loading

Turbo87 commented Oct 16, 2017

siva-sundar commented Oct 17, 2017 •

edited

Loading

Turbo87 left a comment

Turbo87 Oct 28, 2017

Turbo87 commented Feb 5, 2018

siva-sundar commented Feb 7, 2018 •

edited

Loading

Turbo87 commented Feb 8, 2018

siva-sundar commented Feb 8, 2018 •

edited

Loading

Turbo87 Feb 19, 2018

siva-sundar Feb 19, 2018 •

edited

Loading

Turbo87 Feb 19, 2018

siva-sundar Feb 19, 2018 •

edited

Loading

Turbo87 Feb 21, 2018

Turbo87 Feb 19, 2018

siva-sundar Feb 19, 2018 •

edited

Loading

Turbo87 Feb 19, 2018

Turbo87 Feb 19, 2018

Turbo87 Feb 19, 2018

siva-sundar commented Feb 19, 2018 •

edited

Loading

Turbo87 Feb 21, 2018

Turbo87 Feb 21, 2018

siva-sundar Feb 21, 2018

Turbo87 Feb 21, 2018

Turbo87 Feb 21, 2018

Turbo87 Feb 21, 2018

		}


		const canTrimBlockStatementContent = function(node, config) {

Minify Templates based on a white list #14

Minify Templates based on a white list #14

Conversation

siva-sundar commented Sep 29, 2017 • edited Loading

Turbo87 commented Oct 14, 2017

siva-sundar commented Oct 15, 2017 • edited Loading

Turbo87 commented Oct 16, 2017

siva-sundar commented Oct 17, 2017 • edited Loading

Turbo87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Turbo87 commented Feb 5, 2018

siva-sundar commented Feb 7, 2018 • edited Loading

Turbo87 commented Feb 8, 2018

siva-sundar commented Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

siva-sundar Feb 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siva-sundar Feb 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siva-sundar Feb 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siva-sundar commented Feb 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siva-sundar commented Sep 29, 2017 •

edited

Loading

siva-sundar commented Oct 15, 2017 •

edited

Loading

siva-sundar commented Oct 17, 2017 •

edited

Loading

siva-sundar commented Feb 7, 2018 •

edited

Loading

siva-sundar commented Feb 8, 2018 •

edited

Loading

siva-sundar Feb 19, 2018 •

edited

Loading

siva-sundar Feb 19, 2018 •

edited

Loading

siva-sundar Feb 19, 2018 •

edited

Loading

siva-sundar commented Feb 19, 2018 •

edited

Loading