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.

Sign up for GitHub

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

Jump to bottom

WIP: Possible fix for #1252: Using @partial-block twice in a template not possible #1290

Merged
merged 2 commits into from
Feb 14, 2017
Merged

WIP: Possible fix for #1252: Using @partial-block twice in a template not possible #1290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c419c3e
Fix for #1252: Using @partial-block twice in a template not possible
nknapp Dec 30, 2016
81ae953
Add more tests for partial-blocks and inline partials
nknapp Jan 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 14 additions & 10 deletions lib/handlebars/runtime.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,12 +210,7 @@ export function wrapProgram(container, i, fn, data, declaredBlockParams, blockPa
export function resolvePartial(partial, context, options) {
if (!partial) {
if (options.name === '@partial-block') {
let data = options.data;
while (data['partial-block'] === noop) {
data = data._parent;
}
partial = data['partial-block'];
data['partial-block'] = noop;
partial = options.data['partial-block'];
} else {
partial = options.partials[options.name];
}
Expand All @@ -228,6 +223,8 @@ export function resolvePartial(partial, context, options) {
}

export function invokePartial(partial, context, options) {
// Use the current closure context to save the partial-block if this partial
const currentPartialBlock = options.data && options.data['partial-block'];
options.partial = true;
if (options.ids) {
options.data.contextPath = options.ids[0] || options.data.contextPath;
Expand All @@ -236,10 +233,17 @@ export function invokePartial(partial, context, options) {
let partialBlock;
if (options.fn && options.fn !== noop) {
options.data = createFrame(options.data);
partialBlock = options.data['partial-block'] = options.fn;

if (partialBlock.partials) {
options.partials = Utils.extend({}, options.partials, partialBlock.partials);
// Wrapper function to get access to currentPartialBlock from the closure
let fn = options.fn;
partialBlock = options.data['partial-block'] = function partialBlockWrapper(context, options) {
// Restore the partial-block from the closure for the execution of the block
// i.e. the part inside the block of the partial call.
options.data = createFrame(options.data);
options.data['partial-block'] = currentPartialBlock;
return fn(context, options);
};
if (fn.partials) {
options.partials = Utils.extend({}, options.partials, fn.partials);
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, previously, fn === partialBlock. Are you sure that we don't depend on that elsewhere?

Copy link
Collaborator Author

@nknapp nknapp Jan 2, 2017 •
edited

Choose a reason for hiding this comment

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

I have searched the lib-folder and the src-folder for occurences of the string partial-block in the tmp/issue-1252-branch and the only matches are in the functions invokePartial and resolvePartial. Those functions do not check for fn === partialBlock.

But no, I cannot be completely sure. I have just started digging in the internals of Handlebars, there might be something I don't see. I'm pretty confident though. Especially since all tests (including your new ones) are passing.

}
}

Expand Down
95 changes: 95 additions & 0 deletions spec/partials.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,13 +249,46 @@ describe('partials', function() {
true,
'success');
});
it('should be able to render the partial-block twice', function() {
shouldCompileToWithPartials(
'{{#> dude}}success{{/dude}}',
[{}, {}, {dude: '{{> @partial-block }} {{> @partial-block }}'}],
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to see a few more test cases. I can think of at least two that we need:

  • Multiple partial blocks in a nested partial block
    • Given these templates
    <outer>{{#> nested}}<outer-block>{{> @partial-block}} {{> @partial-block}}</outer-block>{{/nested}}</outer>
    <nested>{{> @partial-block}}</nested>
    <template>{{#> outer}}{{value}}{{/outer}}</template>
    • rendering with { value: 'success' } should produce
    <template><outer><nested><outer-block>success success</outer-block></nested></outer></template>
  • Multiple partial blocks at different nesting levels
    • Given these templates
    <outer>{{#> nested}}<outer-block>{{> @partial-block}}</outer-block>{{/nested}}{{> @partial-block}}</outer>
    <nested>{{> @partial-block}}</nested>
    <template>{{#> outer}}{{value}}{{/outer}}</template>
    • rendering with { value: 'success' } should produce
    <template><outer><nested><outer-block>success</outer-block></nested>success</outer></template>

We probably also want to test that inline partials behave as expected when combined with multiple @partial-block invocations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can add those tests.

true,
'success success');
});
it('should render block from partial with context', function() {
shouldCompileToWithPartials(
'{{#> dude}}{{value}}{{/dude}}',
[{context: {value: 'success'}}, {}, {dude: '{{#with context}}{{> @partial-block }}{{/with}}'}],
true,
'success');
});
it('should allow the #each-helper to be used along with partial-blocks', function() {
shouldCompileToWithPartials(
'<template>{{#> list value}}value = {{.}}{{/list}}</template>',
[
{value: ['a', 'b', 'c']},
{},
{
list: '<list>{{#each .}}<item>{{> @partial-block}}</item>{{/each}}</list>'
}
],
true,
'<template><list><item>value = a</item><item>value = b</item><item>value = c</item></list></template>');
});
it('should render block from partial with context (twice)', function() {
shouldCompileToWithPartials(
'{{#> dude}}{{value}}{{/dude}}',
[
{context: {value: 'success'}},
{},
{
dude: '{{#with context}}{{> @partial-block }} {{> @partial-block }}{{/with}}'
}
],
true,
'success success');
});
it('should render block from partial with context', function() {
shouldCompileToWithPartials(
'{{#> dude}}{{../context/value}}{{/dude}}',
Expand Down Expand Up @@ -284,6 +317,50 @@ describe('partials', function() {
true,
'<template><outer><nested><outer-block>success</outer-block></nested></outer></template>');
});
it('should render nested partial blocks at different nesting levels', function() {
shouldCompileToWithPartials(
'<template>{{#> outer}}{{value}}{{/outer}}</template>',
[
{value: 'success'},
{},
{
outer: '<outer>{{#> nested}}<outer-block>{{> @partial-block}}</outer-block>{{/nested}}{{> @partial-block}}</outer>',
nested: '<nested>{{> @partial-block}}</nested>'
}
],
true,
'<template><outer><nested><outer-block>success</outer-block></nested>success</outer></template>');
});
it('should render nested partial blocks at different nesting levels (twice)', function() {
shouldCompileToWithPartials(
'<template>{{#> outer}}{{value}}{{/outer}}</template>',
[
{value: 'success'},
{},
{
outer: '<outer>{{#> nested}}<outer-block>{{> @partial-block}} {{> @partial-block}}</outer-block>{{/nested}}{{> @partial-block}}+{{> @partial-block}}</outer>',
nested: '<nested>{{> @partial-block}}</nested>'
}
],
true,
'<template><outer><nested><outer-block>success success</outer-block></nested>success+success</outer></template>');
});
it('should render nested partial blocks (twice at each level)', function() {
shouldCompileToWithPartials(
'<template>{{#> outer}}{{value}}{{/outer}}</template>',
[
{value: 'success'},
{},
{
outer: '<outer>{{#> nested}}<outer-block>{{> @partial-block}} {{> @partial-block}}</outer-block>{{/nested}}</outer>',
nested: '<nested>{{> @partial-block}}{{> @partial-block}}</nested>'
}
],
true,
'<template><outer>' +
'<nested><outer-block>success success</outer-block><outer-block>success success</outer-block></nested>' +
'</outer></template>');
});
});

describe('inline partials', function() {
Expand Down Expand Up @@ -332,6 +409,24 @@ describe('partials', function() {
true,
'<inner><outer-block>success</outer-block></inner>');
});
it('should render nested inline partials with partial-blocks on different nesting levels', function() {
shouldCompileToWithPartials(
'{{#*inline "outer"}}{{#>inner}}<outer-block>{{>@partial-block}}</outer-block>{{/inner}}{{>@partial-block}}{{/inline}}' +
'{{#*inline "inner"}}<inner>{{>@partial-block}}</inner>{{/inline}}' +
'{{#>outer}}{{value}}{{/outer}}',
[{value: 'success'}, {}, {}],
true,
'<inner><outer-block>success</outer-block></inner>success');
});
it('should render nested inline partials (twice at each level)', function() {
shouldCompileToWithPartials(
'{{#*inline "outer"}}{{#>inner}}<outer-block>{{>@partial-block}} {{>@partial-block}}</outer-block>{{/inner}}{{/inline}}' +
'{{#*inline "inner"}}<inner>{{>@partial-block}}{{>@partial-block}}</inner>{{/inline}}' +
'{{#>outer}}{{value}}{{/outer}}',
[{value: 'success'}, {}, {}],
true,
'<inner><outer-block>success success</outer-block><outer-block>success success</outer-block></inner>');
});
});

it('should pass compiler flags', function() {
Expand Down