search

handlebars-lang / handlebars.js

Minimal templating on steroids.
http://handlebarsjs.com
MIT License
17.99k stars 2.04k forks source link

Disable new default auto-indent at included partials #858

Closed majodev closed 9 years ago

majodev commented 10 years ago

Handlebars >v2.0.0-alpha.4 introduced a new default indentation handling at partials, namely "Partials that are standalone will now indent their rendered content" (source). This might cause unexpected behavior when upgrading from previous versions, is there a flag to restore the old handling?

Detailed explanation / self reproduce

Thanks in advance! :)

kpdecker commented 10 years ago

What sort of failures have you seen? It's not too hard to implement a flag for this but I want to make sure I understand the failure case.

majodev commented 10 years ago

It's not a failure by itself, I only think such a flag might be convenient for compiling partials/templates, that depend on the old handling.

Use case: A partial that inserts content, which holds transformed 'Markdown to HTML'-content and has code tags within it. The content within these code tags has newlines and these lines should not get automatically intended to the outer template's indentation level, where the partial gets inserted.

Of course it's rather easy to workaround this problem by inserting every partial that requires the old handling at zero indentation in outer template, however, I thought such a flag might be convenient for folks, who want to upgrade to newer Handlebars versions but rely on the old indentation handling and don't want to modify their templates after updating.

Anyways, I'm unsure if the old handling was compliant to the Mustache spec, so it might be better to just accept it as it is now and close my issue. :)

joshterrell805 commented 10 years ago

Verification: I have exactly this problem right now.

helper '/posts/post'

<h1 class='title'>{{title}}</h1>
<div class='body'>{{{body}}}</div>

main template

<body>
   {{> '/header'}}
   <div class='post row small-11 small-centered columns'>
      {{> '/posts/post' this}}
   </div>
   {{> '/footer'}}
</body>

rendered html

   <div class='post row small-11 small-centered columns'>
      <h1 class='title'>Content Indexing</h1>
      <div class='body'><h3 id="problem">Problem</h3>
      <p>I want to build an index page which shows the latest <code>X</code> posts. I need a function which returns an <code>index</code> of the posts.</p>
      <pre><code class="lang-js">index = [
         &quot;newest post&#39;s title&quot;,
         &quot;second-newest post&#39;s title&quot;,
         ...
      ];
      </code></pre>

Screwed up indentation in displayed code: image

kevinrenskers commented 10 years ago

I had the exact same issue too: https://github.com/kevinrenskers/raml2html/issues/66

75lb commented 10 years ago

this auto-indent feature should be optional because it completely breaks generation of whitespace-sensitive output like

My templates generates a lot of the above so i'm currently i'm stuck on v2.0.0-alpha.4 until this flag lands on master..

shenanigans commented 10 years ago

I, too, need this feature. How much of a pain would it be to specify that a single 'stache replacement be unindented, using a syntax cue of some kind? i.e. {{|> preformattedHelper}}

ErisDS commented 9 years ago

Ghost users are also suffering from issues with the auto-indentation. Here's a forum thread where we figured out that this was the cause of a problem a user was seeing with syntax highlighting on their Ghost blog.

Essentially, the auto indent causes extra indentation to be added to any <pre> blocks inside the partial, so the content of the <pre> is rendered WITH the extra indentation.

I guess it would help to submit a PR with a failing test or something for this?

majodev commented 9 years ago

@ErisDS: The problem is, that it would make no sense to explicitly exclude pre blocks from this new indentation handling within the handlebars library, as the engine simply shouldn't make these assumptions (and handlebars is not limited to work with HTML blocks only). However, the addition of a test would be excellent to secure this handling in the future.

I like the idea of @shenanigans' {{|> preformattedHelper}} to disable the feature at a block level, but would also love to see a global option, so former templates that rely on the old behavior work as expected again by setting a single flag (e.g. within the complile method).

@kpdecker I really feel this issue reached a critical mass when it comes to backward compatibility. I'm aware of the long discussions this new indentation handling has caused in former pull requests, it's a tough problem as people really have a different opinion on how this should work by default. Is this new behavior dictated by the mustache spec (#811)?

ErisDS commented 9 years ago

I understand completely and wouldn't expect that the behaviour should apply differently to <pre> blocks. I just wanted to come along with a real world example of how this change is causing hard to diagnose bugs in existing software.

I think for our use case, a global flag makes most sense, as I think we'd want to disable this feature across all themes. Trying to communicate this subtlety to theme developers would be incredibly difficult, and I'm not sure what benefit the indentation provides in themes other than clean source.

ErisDS commented 9 years ago

Here's a JSFiddle that demonstrates the change in behaviour:

Latest Handlebars: http://jsfiddle.net/6d7wbfna/1/ Handlebars 1.3.0: http://jsfiddle.net/6d7wbfna/

ErisDS commented 9 years ago

I'm left wondering why this behaviour was changed in the first place?

As far as I can tell, it was purely for aesthetic reasons (please correct me if I am wrong). I appreciate clean source as much as the next person, but this has fundamentally changed the mapping between handlebars and HTML far beyond an aesthetic source code level improvement.

Although there are many possible representations of any given piece of HTML with variations of whitespace that are visually (and functionally) equivalent, this has changed the output from handlebars to use a representation of HTML that is no longer visually or functionally equivalent to the previous version. This is, therefore, a breaking change, and I think warrants further thought beyond just putting in any sort of flag.

This change forces every person who is crafting templates in handlebars to consider the whitespace within the entire tree of nested partials in order to determine if the output from a <pre> or <textarea> tag will render as expected. In short, we can no longer reliably determine what HTML Handlebars will output.

If the only reason for this change is an aesthetic one, then I believe it's worth considering reverting the change.

The comment by @wycats here (I know it was from a while ago) expressed this concern, including the problem that doing this with or without a flag would cause, and I'd like to reiterate that concern.

kpdecker commented 9 years ago

This feature was changed in order to match the mustache spec, which another subset of users was requesting while 2.x was being developed. We are in a position where every single change made or not to the language will piss someone off.

Regarding pre comments: Handlebars is agnostic to the output. It's not aware of what language is being output, as @majodev noted.

Block level: Does whitespace control prevent indentation? It seems to me that it should and if it does not then it's a bug that we should fix. If someone could confirm this for me that would be awesome as I am swamped with the day job.

Complier config option: It would not be too hard to implement such a behavior, simply adding an options check to utilize or omit indent's value here. My only concern here is that with each flag that we introduce at the global level the rendering behavior becomes far less predictable IMO, but I also am not using a large mass of nested partials in my own projects so I could see how this issue could compound on itself.

joshterrell805 commented 9 years ago

The indentation is a problem in pre formated blocks. To put a bandaid on this problem, I've been including all helpers at the 0 indent column (no matter what the parent block of the line is).

As you can imagine this looks terrible and breaks readability.

Does anyone have any other ideas except the two ideas recommended thus far? (compiler flag and special syntax)

majodev commented 9 years ago

@joshterrell805 nope, I've done the same within my metalsmith pipeline (compiled Markdown content gets inserted into a HTML unescaped handlebars expression), looks awful, but works.

ErisDS commented 9 years ago

We are in a position where every single change made or not to the language will piss someone off.

@kpdecker I get it, honestly I do, you literally cannot please everyone! Clearly though, someone made a case strongly enough to get this changed, so there's always the possibility there's an equal and opposite case to change it back that hasn't been heard yet ;)

What I'm trying to get across is that there's a big difference between an aesthetic and a functional change, and it seems like this change was possibly made without the functional ramifications being clear?

Handlebars is agnostic to the output. It's not aware of what language is being output, as @majodev noted.

Handlebars might be output agnostic, but it is frequently used to output languages for which whitespace has meaning, and so changing how whitespace is output is never going to be 100% safe.

Anyway - to continue the discussion towards at the very least having a compiler flag, I believe that handlebars is commonly used in situations where it cannot be expected that the person writing templates has this sort of fine-grained understanding of what an indent could do further down the partial stack. This introduces subtle issues which are hard to diagnose and therefore I'm very much in favour of at least having an option to switch it off. Pretty please :cake:

joshterrell805 commented 9 years ago

It's clear that this is a functional problem that needs to be addressed.

I think many of us would be willing to create a pull to fix this issue, but which solution should be implemented? compiler flag? special include syntax? other? all?

shenanigans commented 9 years ago

@joshterrell805 I eventually took my helpers off the first column and put their preformatted sections in tags without newlines. <pre>{{{markdown}}}</pre>. Then I slapped myself in the face real hard.

joshterrell805 commented 9 years ago

Then I slapped myself in the face real hard.

lol! There's plenty of situations where the pre tags need to be in the helpers, though.

shenanigans commented 9 years ago

They are in the helpers partials. The helper's partial's position may cause the pre-tag itself to be indented unreliably, but as long as the tag contents are correct it will render properly. The contents of the replacement are not given extra indentation so you only need to avoid adding any extraneous newlines and voila.

actual helpers are fine with triplestache. <pre>{{{markdown docStr}}}</pre> but I believe not fine with doublestache.

joshterrell805 commented 9 years ago

Thanks for the correction; I meant partial too, not helper.

The contents of the replacement are not given extra indentation so you only need to avoid adding any extraneous newlines and voila.

What qualifies as extraneous newlines? In the method you mentioned above (<pre>{{{markdown docStr}}}</pre>), can docStr contain newlines and retain indentation specified in docStr only? From my experimentation this was not the case...all newlines in docStr get indented by whatever the indentation of the pre block is.

If docStr can contain newlines and retain the specified indentation (not inheriting the indentation of the pre-block), this is a valid solution to the problem, and I don't think any new code needs to be written.

shenanigans commented 9 years ago

docStr can contain newlines without tripping indentation behavior as long as the replacement is triplestache, not double. Technically, in the example you're worried about the return value of markdown, not the contents of docStr but same thing. If you need to escape html but you still need to avoid indentation, use a helper and call Handlebars.Utils.escapeExpression.

In my current project, I use the exact line <div class="markdown">{{{markdown docStr}}}</div> with markdown often returning a <pre> as part of its output.

joshterrell805 commented 9 years ago

docStr can contain newlines without tripping indentation behavior as long as the replacement is triplestache, not double. ...In my current project, I use the exact line ...

This solves my problem. I'll try this out the next time I'm working on display code. Thanks so much @shenanigans!

kpdecker commented 9 years ago

I've added a test that confirmed the correct behavior when using whitespace control and partials. I've also added a preventIndent option to the compiler that when enabled will not cause indents when rendering any partial.

ericlowry commented 9 years ago

Sorry if this is obvious - but how do I enable preventIndent ? I am using express-handlebars and I want to set the option for all partials... there must be a way. This is super frustrating.

kpdecker commented 9 years ago

@ericlowry this is a compiler option. I'm not sure how those are set when using express-handlebars. I would consult their documentation or the project owners.

ericlowry commented 9 years ago

Thanks, I guess I was hoping that someone would have encountered this and had a ready solution.

I have an reference to the actual handlebars instance used inside of handlebars express. Is there a way to override the default compiler options? I figure I must be missing something obvious...

handlebars.compiler.options.preventIndent = true; // <-- something like this?

kpdecker commented 9 years ago

You need to find the point where the compile or preform oiler call is made. It's an argument to that. On Fri, Dec 5, 2014 at 8:37 AM Eric Lowry notifications@github.com wrote:

Thanks, I guess I was hoping that someone would have encountered this and had a ready solution.

I have an reference to the actual handlebars instance used inside of handlebars express. Is there a way to override the default compiler options? I figure I must be missing something obvious...

handlebars.compiler.options.preventIndent = true; // <-- something like this?

— Reply to this email directly or view it on GitHub https://github.com/wycats/handlebars.js/issues/858#issuecomment-65797517 .

mgartner commented 9 years ago

@kpdecker why hasn't a 2.0.x patch been released with the new preventIndent option? The only option is bleeding edge or downgrading to 1.3.0 where this isn't an issue.

ericlowry commented 9 years ago

I agree completely... release the patch already.

Eric Lowry eric@myrealms.org http://ericmlowry.com (801) 747-9758

On Sat, Dec 20, 2014 at 9:48 PM, Marcus Gartner notifications@github.com wrote:

@kpdecker https://github.com/kpdecker why hasn't a 2.0.x patch been released with the new preventIndent option? The only option is bleeding edge or 1.3.0 it seems like.

— Reply to this email directly or view it on GitHub https://github.com/wycats/handlebars.js/issues/858#issuecomment-67760080 .

kpdecker commented 9 years ago

This is targeted for the 3.0 release as noted by the milestone attached to this bug. There are too many changes on the tree right now to release anything on the 2.x codeline, at least not without significant time input that I don't have right now. I hope to have 3.0 out soonish, but the job that pays me as well as family obligations are superseding that right now.

lzilioli commented 9 years ago

This same behavior appears to exist when calling partials, or outputting content with the {{{ }}} syntax (as described by @joshterrell805). Is there a way to prevent this behavior in these cases as well?

kpdecker commented 9 years ago

@lzilioli please open a new issue with supporting fiddle or other repo case if you're running into issues. I'm not sure what you mean by the same behavior as there are a few things discussed here, quite awhile ago, and it's probably better to start a new standalone conversation if you have concerns.