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

Add an option to ignore all unneeded whitespace #51

Closed
TimothyGu opened this issue Jan 26, 2015 · 15 comments · Fixed by #56
Closed

Add an option to ignore all unneeded whitespace #51

TimothyGu opened this issue Jan 26, 2015 · 15 comments · Fixed by #56
Assignees
Labels
enhancement New feature or request

Comments

@TimothyGu
Copy link
Collaborator

Benchmarking shows that sometimes too many

__output += "        \n";

will have a very negative effect on performance. It is also one of the major reasons why doT is much faster than EJS, even though theoretically EJS is simpler.

New line slurping does get rid of the new line, but then you still have

__output += "        ";
@TimothyGu TimothyGu added the enhancement New feature or request label Jan 26, 2015
@Fonger
Copy link

Fonger commented Jan 28, 2015

@TimothyGu said

We really want to keep EJS small and maintainable. You can always hook EJS output up to some html prettifier easily yourself.

I suggest EJS provide an intermediate function to help parsing EJS output.
In production mode, static portion will be cached in memory in EJS.
It is best to cache the minified version in EJS so I think providing an intermediate function to process the output html will be a great idea. In that way, you can implement your own minifying function there with maximum performance.

For example,

app.set('views', path.join(__dirname, 'views'));
app.set('view engine', 'ejs');
app.use(ejs({intermediate: function(input) {
    return minify(input); //Implement your own minify function here and ejs will cached this version.
}})

@TimothyGu
Copy link
Collaborator Author

@Fonger

You could do something like this (untested):

var ejs = require('ejs')
app.engine('ejs', function (filePath, options, callback) {
  ejs.__express(filePath, options, function (err, html) {
    if (err) return callback(err)
    callback(minify(html))
  }
})
app.set('views', path.join(__dirname, 'views'))
app.set('view engine', 'ejs')

@Fonger
Copy link

Fonger commented Jan 29, 2015

@TimothyGu
It's not working.
Somehow it renders my error page with raw html output.
And console.log(html) shows that html is already rendered(compiled) with variable.

app.engine('ejs', function (filePath, options, callback) {
  ejs.__express(filePath, options, function (err, html) {
    if (err) return callback(err);
    console.log(html);
    callback(html);
  });
});
// view engine setup
app.set('views', path.join(__dirname, 'views'));
app.set('view engine', 'ejs');

@TimothyGu
Copy link
Collaborator Author

@Fonger said:

And console.log(html) shows that html is already rendered(compiled) with variable.

This is expected. In the Express.js world, options contains both options and locals.

@Fonger
Copy link

Fonger commented Jan 29, 2015

I find the following code working!
And I've tested and confirmed that if you run in production mode, parse function will only trigger once when the page loads for the first time.

Furthermore, html_ejs here is not rendered(compiled) yet. The content is raw ejs html, which means that it contains <%=someVariable%> unlike the previous problem I mentioned.

Source: ejs-shrink

var ejs = require('ejs');

var parse = ejs.parse;
ejs.parse = function(html_ejs, options) {
    html_ejs = html_ejs.replace(/^\s+|\s+$|\n/gm,"");
    console.log('parse!!!!!!'); // to check if it only runs once.
    return parse.apply(this, [html_ejs, options]);
};

I don't know why the author of ejs-shrink deleted the repo on GitHub. However it's still accessible via npm install ejs-shrink. It's so outdated and even wants node 0.6.x. However it's working though.

And this is the forked one.
https://github.com/sylvinus/ejs-shrink/

@TimothyGu
Copy link
Collaborator Author

OK this works:

var ejs = require('ejs')
app.engine('ejs', function (filePath, options, callback) {
  ejs.__express(filePath, options, function (err, html) {
    if (err) return callback(err)
    callback(null, minify(html))
  })
})
app.set('views', path.join(__dirname, 'views'))
app.set('view engine', 'ejs')

Forgot about the first argument to the callback.

EDIT: add missing )

@TimothyGu
Copy link
Collaborator Author

@Fonger said:

And this is the forked one.

https://github.com/sylvinus/ejs-shrink/

From the Git history we can see that the original regex is /<!--[\s\S]*?-->/g plus line trimming. But then @sechrist removed comment dropping and started using /^\s+|\s+$|\n/gm to match whitespace.

@Fonger
Copy link

Fonger commented Jan 29, 2015

@TimothyGu You still miss one ). I've tested your code it's working however they're not cached so it has performance issue. Your method will minify the final html output while ejs-shrink will minify the original ejs file and so they're cached once loaded.

TimothyGu added a commit that referenced this issue Jan 29, 2015
This mode uses a regex to drop

1. line feeds
2. carriage returns
3. leading whitespaces of lines
4. trailing whitespaces

It is not a full-fledged HTML minifier, nor will it be.

Fixes #51.
@TimothyGu
Copy link
Collaborator Author

@Fonger said:

You still miss one ')'.

Oops, post updated.

I've tested your code it's working however they're not cached so it has performance issue.

Yes that is intended.

I have committed 6c12c2e to the drop-whitespace branch of EJS, which incorporates some changes from ejs-shrink, but adapted to EJS v2, so feel free to try it out.

ejs-shrink only works on EJS v1 as we don't have a parse() in EJS v2.

@Fonger
Copy link

Fonger commented Jan 29, 2015

@TimothyGu said:

ejs-shrink only works on EJS v1 as we don't have a parse() in EJS v2.

Oops! I didn't find that I was still using old EJS v1.
Is there any big improvement from v1 to v2 ?

@TimothyGu
Copy link
Collaborator Author

@Fonger said:

Is there any big improvement from v1 to v2 ?

See https://github.com/mde/ejs/blob/master/CHANGELOG.md#v201-2015-01-02

EJS v2 is a complete rewrite of v1, using OOP and regex rather than one big parse() and String.prototype.indexOf(). Filters were removed, and some APIs were changed.

One negative thing about EJS v2 is that it's much slower than EJS v1, both in terms of compilation and execution. I've already made EJS v2 4 times faster than prior to my involvement in the project, and #50 should give it an additional 30-80% boost, although it is still slower than EJS v1.

Note that I have a fork of EJS that uses EJS v1 code base but ported all the v2 features over, but all API changes and features are discussed here as I intend to give my fork 100% compatibility with EJS v2.

If you get a chance please test drop-whitespace branch of EJS v2 and let me know what you think (and if it works at all...)

@Fonger
Copy link

Fonger commented Jan 29, 2015

@TimothyGu Thank you.
I don't know how to config the options when using render with expressjs.

    res.render('welcome',
        { myVariable: 123 }, {removeWhitespace:true});

The above code doesn't work. It says TypeError: object is not a function.
Is it possible to set a global default option for ejs?

@TimothyGu
Copy link
Collaborator Author

@Fonger:

The easiest way to test EJS is simply cloning it and test locally:

$ git clone https://github.com/mde/ejs.git
$ git checkout drop-whitespace
$ cd ejs
$ npm install
$ edit 'test.js'
// test.js
var ejs = require('./')
  , str = '<%= myVariable %>\t\r\n         adfs   \n\t asdf'

console.log(ejs.render(str, { myVariable: 123 }, { removeWhitespace: true }))

But if you really want to test with Express.js, put locals and options in to a single object, like this:

res.render('welcome', {
  myVariable: 123
, removeWhitespace: true
})

Note that this behavior might change in Express.js version 5.

TimothyGu added a commit that referenced this issue Feb 1, 2015
It is not a full-fledged HTML minifier, nor will it be. It is designed to
be as "safe" as possible.

Fixes #51.
TimothyGu added a commit that referenced this issue Feb 1, 2015
It is not a full-fledged HTML minifier, nor will it be. It is designed to
be as "safe" as possible.

Fixes #51.
TimothyGu added a commit that referenced this issue Feb 1, 2015
It is not a full-fledged HTML minifier, nor will it be. It is designed to
be as "safe" as possible.

Fixes #51.
@TimothyGu TimothyGu self-assigned this Feb 3, 2015
TimothyGu added a commit that referenced this issue Feb 6, 2015
It is not a full-fledged HTML minifier, nor will it be. It is designed to
be as "safe" as possible.

Fixes #51.
TimothyGu added a commit that referenced this issue Feb 7, 2015
It is not a full-fledged HTML minifier, nor will it be. It is designed to
be as "safe" as possible.

Fixes #51.
TimothyGu added a commit that referenced this issue Feb 7, 2015
It is not a full-fledged HTML minifier, nor will it be. It is designed to
be as "safe" as possible.

Fixes #51.
TimothyGu added a commit that referenced this issue Feb 7, 2015
It is not a full-fledged HTML minifier, nor will it be. It is designed to
be as "safe" as possible.

Fixes #51.
TimothyGu added a commit that referenced this issue Feb 14, 2015
It is not a full-fledged HTML minifier, nor will it be. It is designed to
be as "safe" as possible.

Fixes #51.
@MrSwitch
Copy link

MrSwitch commented Jul 1, 2016

FYI: I found a way to define this in ExpressJS app

i.e.

app.set('view engine', 'ejs');
app.locals.rmWhitespace = true;

@diamont1001
Copy link

nice, thanks

Repository owner locked as resolved and limited conversation to collaborators Jan 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants