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

Render blocks in the parent context and assure a block is passed to templates that use @partial-block #620

Merged
merged 9 commits into from
May 24, 2018

Conversation

nyon
Copy link
Contributor

@nyon nyon commented May 3, 2018

We heavily use @partial-block in our project context. We noticed the following flaws with the current evaluation of partial-blocks in handlebars.java:

  • If a partial provides a block, it is currently evaluated twice. We added an option to only lazy evaluate the partial to improve performance.
  • Currently the implementation of @partial-block throws a stack overflow error (infinite recursion), when a block of a partial provides a template which also contains @partial-block (see example below). This is fixed by this pull request. Handlebars.js also supports this, see WIP: Possible fix for #1252: Using @partial-block twice in a template not possible handlebars-lang/handlebars.js#1290
  • In order to protect against the case when a partial-block is required but not provided, we introduced the callee. It is used to check whether the last provided partial-block is the right partial block for the given call to {{> @partial-block}}.

stack overflow example:

{{!-- page.hbs --}}
{{#> special-button}}
  Lorem ipsum
{{/special-button}}

{{!-- special-button.hbs --}}
{{#> generic-button}}
  {{> @partial-block}}
{{/generic-button}}

{{!-- generic-button.hbs --}}
<button>
  {{> @partial-block}}
</button>

@nyon nyon changed the title Render blocks in the parent context and assure a block is passed to templates that use @partial-block [WIP] Render blocks in the parent context and assure a block is passed to templates that use @partial-block May 3, 2018
@coveralls
Copy link

coveralls commented May 4, 2018

Coverage Status

Coverage increased (+0.04%) to 86.252% when pulling 94f37c0 on nyon:master into 1f6c48e on jknack:master.

@nyon nyon changed the title [WIP] Render blocks in the parent context and assure a block is passed to templates that use @partial-block Render blocks in the parent context and assure a block is passed to templates that use @partial-block May 7, 2018
Copy link
Owner

@jknack jknack left a comment

Choose a reason for hiding this comment

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

Looks good, please add more of tests with lazyPartialBlockEvaluation = true

@@ -740,6 +746,39 @@ public boolean stringParams() {
return stringParams;
}


/**
* If true, given partial blocks are not evaluated when defined but when used.
Copy link
Owner

Choose a reason for hiding this comment

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

Let's add two examples when this flag is on/off

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey I added two tests that deal with this pull request's changes. Is everything okay now?

}

Template template = inlineTemplates.get(path);


Copy link
Owner

Choose a reason for hiding this comment

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

remove empty line

@@ -171,18 +196,37 @@ protected void merge(final Context context, final Writer writer)
}

}
context.data("callee", this);
Copy link
Owner

Choose a reason for hiding this comment

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

This is something internal and it shouldn't be called callee. Let's create a name which is hard to override by users like Context. INVOCATION_STACK or/and by prefixing the class name: com.github.jknack.handlebars.internal.Partial.calle

@@ -191,7 +235,7 @@ protected void merge(final Context context, final Writer writer)
* @return True, if the file was already processed.
*/
private static boolean exists(final List<TemplateSource> invocationStack,
final String filename) {
final String filename) {
Copy link
Owner

Choose a reason for hiding this comment

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

please revert all the whitespace changes

@jknack jknack added this to the 4.0.7 milestone May 24, 2018
@jknack jknack merged commit dc4fb61 into jknack:master May 24, 2018
final boolean parentIsNotLastPartialBlock = !isCalleeOf(callee, lastPartialBlock);

if (pathIsPartialBlock && parentIsNotLastPartialBlock) {
throw new IllegalArgumentException(
Copy link
Owner

@jknack jknack May 24, 2018

Choose a reason for hiding this comment

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

Already merged it, but I'm struggling to figure out why we do fail here, can you explain?

Here is a test case that fails today bc of this check:

shouldCompileTo("{{#> dude x=23}}{{#> dude x=12}}{{/dude}}{{/dude}}",
        $("hash", $(), "partials",
            $("dude", "<div {{#if x}}x={{x}}{{/if}}>{{> @partial-block}}</div>")),
        "<div x=23><div x=12></div></div>");

Choose a reason for hiding this comment

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

Hi and thanks for the merge! We'll get back to you about this asap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants