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

[feature] include should preserve indention #176

Open
camerondavison opened this issue Jun 9, 2016 · 17 comments
Open

[feature] include should preserve indention #176

camerondavison opened this issue Jun 9, 2016 · 17 comments
Labels
enhancement New feature or request

Comments

@camerondavison
Copy link

If someone does

<p>
  <%- include('other.ejs') %>
</p>

and other.ejs is

<div>
  <b>HELLO</b>
</div>

Then it should look like

<p>
  <div>
    <b>HELLO</b>
  </div>
</p>

Instead of

<p>
  <div>
  <b>HELLO</b>
</div>
</p>
@RyanZim
Copy link
Collaborator

RyanZim commented Jun 10, 2016

I suppose that would be nice, but I can't think of a sane way to do it (unless you can). For now, your best option is to use an html beautifier like htmltidy or beautifier.

CC: @mde @TimothyGu

@camerondavison
Copy link
Author

In this specific example it could be run through some post processing, but not say if it were python or yaml which care about whitespace. Since yeoman is using this to help generate code, probably would be nice to support this. I can try and look through the code to see if I see a way to do it.

@camerondavison
Copy link
Author

Look through it seems like its just a matter of passing the indention of the < for the include to the next function. Then before printing the string, split on '\n' and prepend the indention to every line.

@RyanZim RyanZim added the enhancement New feature or request label Jun 10, 2016
@RyanZim
Copy link
Collaborator

RyanZim commented Jun 10, 2016

I'll let @mde & @TimothyGu decide if this is something to implement.

@TimothyGu
Copy link
Collaborator

EJS was designed to be barebones, without the bells and whistles. That gives it its performance, but yes, it does mean that features like indentation have to go. Think of EJS as a C preprocessor if you are familiar with how it works, that gets its job done but gives ugly output.

@a86c6f7964, to solve your immediate problem, if you have control over the contents of other.ejs, you should indent there, and unindent the include line.

Aside from that, honestly I don't believe that Yeoman's choice of using EJS is the best one, since Yeoman doesn't care about performance (nor should it), and there are many other template engines out there that are slower, but give better looking output, etc.

@SBoudrias
Copy link

Aside from that, honestly I don't believe that Yeoman's choice of using EJS is the best one, since Yeoman doesn't care about performance (nor should it), and there are many other template engines out there that are slower, but give better looking output, etc.

FWIW, the reason was keeping compatibility with underscore templates.

Also, I don't agree ejs doesn't care about indentation (also for performance reason is total bollocks come on). Right now you have a ton of whitespace and indentation related features like whitespace trim or the slurp mode.

So maybe you don't want to support that feature - and that's totally fine. But don't go on saying ejs as a template language doesn't care about the output as this is false.

@RyanZim
Copy link
Collaborator

RyanZim commented Jun 13, 2016

BTW, you could also write your own function, like this:

<%
function indent(text, depth) {
  var output='';
  text.split("\n").foreach(function(line, num){
    // To avoid indenting the first line
    if(num>0){
      for(var i=0;i<depth;i++){
        line=line+' ';
      }
    }
    output+=line;
  });
}
-%>

Then you could do something like <%- indent(include('filename'), 4) %> if you wanted the include to be indented 4 spaces.

@TimothyGu
Copy link
Collaborator

@SBoudrias said:

FWIW, the reason was keeping compatibility with underscore templates.

Hmm. #55 (comment).

Also, I don't agree ejs doesn't care about indentation (also for performance reason is total bollocks come on). Right now you have a ton of whitespace and indentation related features like whitespace trim or the slurp mode.

Just wanted to apologize for my tone in my last comment. I just realized that EJS right now was very different from the EJS I worked on a year ago.

A year ago, I was optimizing EJS as if it was a finished product, i.e. literally to the last inch, including replace costly .replace() calls with less costly ones and with even less ones. At the end, the template performance was on par with the fastest template engines out there, but the compilation performance still not so.

I approached this issue as if EJS still cared about compilation performance, but seems like we don't any more. More specifically, features like <%_ _%> came along that basically added to the already slow compilation process with a bunch of unconditional .replace calls.

So again, I apologize. EJS has become something I didn't think it was, and that anachronism in my head led to my comment.

On a more technical side, yes, those features might be doable, but will be hard to implement nevertheless. If dynamic inclusion is used, then a run-time solution like the one posed by @RyanZim would be much easier to use.

@TimothyGu TimothyGu reopened this Jun 14, 2016
@RyanZim
Copy link
Collaborator

RyanZim commented Jun 20, 2016

@mde thoughts?

@camerondavison
Copy link
Author

camerondavison commented Jul 11, 2016

if you have control over the contents of other.ejs, you should indent there, and unindent the include line.

This is not practical, since I was hoping to write something that would be genric and used in multiple places (hence is being included otherwise what is the point) and the indention is different in different places.

Then you could do something like <%- indent(include('filename'), 4) %>

The fact that it is seemingly okay to be able to write something like this, should imply to me that it should be supported by the library. Essentially all I am asking is that ejs do that function be default. I am not super excited about having to keep track of the number of spaces I want something indented in the code (seems like a fair work around) but seems super easy to mess up.

@RyanZim
Copy link
Collaborator

RyanZim commented Jul 11, 2016

it should be supported by the library

Unless @mde disagrees, we would be open to a PR. It would be best to expose this functionality behind a flag IMO. @mde?

CC: @TimothyGu

@canastro
Copy link

canastro commented Jan 5, 2017

This is super important when using jade 😄

@RyanZim
Copy link
Collaborator

RyanZim commented Jan 5, 2017

@canastro Care to contribute?

@canastro
Copy link

canastro commented Jan 5, 2017

I'm sorry at the moment I don't have the time to understand the codebase and fix this issue 😟

@allergeek
Copy link

Hi, I'd really love to have the indents in place, too. Thanks a lot!

@Pimm
Copy link

Pimm commented Nov 16, 2017

I had the same thought as @a86c6f7964: I wanted indentation. @RyanZim's idea for an indent function came to mind. But instead of adding a function, I chose to re-implement include.

Note that my code runs on Node.js or an environment which provides similar APIs.

const path = require('path');
const fileSystem = require('fs');
const ejs = require('ejs');

function render(outerTemplatePath, data) {
	// Steal the "path base" from the passed template path, create a function which resolves paths based on that base, and
	// make the passed template path relative to it (overwriting the argument).
	const resolveTemplatePath = (function defineResolveTemplatePath() {
		const templatePathsBase = path.dirname(outerTemplatePath);
		outerTemplatePath = path.relative(templatePathsBase, outerTemplatePath);
		return path.resolve.bind(path, templatePathsBase);
	})();
	// (Define the options which are passed to readFileSync.)
	const utf8EncodingOptions = {
		encoding: 'utf8'
	};
	// Define the regular expression used to find newlines while applying indentation.
	const newLineMatcher = /\n/g;
	// Define the include function. This function loads the templates and feeds them through ejs.
	function include(templatePath, indentation) {
		// Load the template.
		var template;
		try {
			template = fileSystem.readFileSync(
				resolveTemplatePath(templatePath),
				utf8EncodingOptions
			);
		} catch (error) {
			throw new Error([
				['Unable to load template: ', templatePath].join(''),
				['Note that the path to the template is resolved relative to the path of this template: ', outerTemplatePath].join(''),
				error.message
			].join('\n'));
		}
		// Render using ejs.
		var result;
		try {
			result = ejs.render(
				template,
				data
			);
		} catch (error) {
			throw new Error([
				['Unable to render template: ', templatePath].join(''),
				error.message
			].join('\n'));
		}
		// Trim the ejs output.
		result = result.trim();
		// Apply the indentation.
		if (undefined !== indentation) {
			result = result.replace(
				newLineMatcher,
				'\n' + indentation
			);
		}
		return result;
	}
	// Inject the include function into the passed data. This makes the function available for use in the templates. (Note
	// that this the if overwrites the argument; while the else alters it. The latter might cause issues if someone passes
	// a long-lived object as data.)
	if (undefined === data) {
		data = {
			include
		};
	} else /* if (undefined !== data) */ {
		Object.assign(
			data,
			{
				include
			}
		);
	}
	// Include the outer template.
	return include(outerTemplatePath, undefined);
}

The original example (above) would become this (note the second argument passed to include):

<p>
  <%- include('other.ejs', '  ') %>
</p>

I am not very familiar with the inner workings of ejs and not at all with its design philosophy, so this might be silly. But it currently works for me, and it might work for the next person to Google Search themselves here.

If anyone does end up copying this snippet, note that there's a trim call (line 49-ish) which you may or may not want.

@jeffory-orrok
Copy link

BTW, you could also write your own function, like this:

<%
function indent(text, depth) {
  var output='';
  text.split("\n").foreach(function(line, num){
    // To avoid indenting the first line
    if(num>0){
      for(var i=0;i<depth;i++){
        line=line+' ';
      }
    }
    output+=line;
  });
}
-%>

Then you could do something like <%- indent(include('filename'), 4) %> if you wanted the include to be indented 4 spaces.

Not sure if there's any value in pointing out issues with a five year old comment, but the above function will not produce desired output for a few reasons: 1) you've lost all your newlines, 2) the spaces are being added to the ends of the lines, and 3) unless there's some magic in the ejs compiler I've overlooked, nothing is actually returned.

The following will indent non-empty lines
<% function indent(text, depth) { return text.split('\n').map(line => (line.length ? ' '.repeat(depth) : '') + line).join('\n'); } %>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

8 participants