Walk up data frames for nested @partial-block #1243
Conversation
The root cause of handlebars-lang#1218 is that `invokePartial` creates a stack of data frames for nested partial blocks, but `resolvePartial` always uses the value at top of the stack without "popping" it. The result is an infinite recursive loop, as references to `@partial-block` in the partial at the top of the stack resolve to itself. So, walk up the stack of data frames when evaluating. This is accomplished by 1) setting the `partial-block` property to `noop` after use and 2) using `_parent['partial-block']` if `partial-block` is `noop` Fix handlebars-lang#1218
| @@ -197,7 +197,12 @@ export function wrapProgram(container, i, fn, data, declaredBlockParams, blockPa | |||
| export function resolvePartial(partial, context, options) { | |||
| if (!partial) { | |||
| if (options.name === '@partial-block') { | |||
| partial = options.data['partial-block']; | |||
| let data = options.data; | |||
| while (data['partial-block'] === noop) { | |||
There was a problem hiding this comment.
This loop is a bit scary, I know. I'm open to suggestions for a base case to bail on.
There was a problem hiding this comment.
Can you explain where noop comes from?
There was a problem hiding this comment.
I.e. what causes it to be populated in the first place.
There was a problem hiding this comment.
After I set partial to data['partial-block'], I set data['partial-block'] to noop on line 205 so that we'll know it's been used the next time we're in here.
|
I just verified that this also fixes the reduced test cases provided by @damncabbage and @stringbean in #1185. |
|
@kpdecker I know you don't have any time for Handlebars right now. Unfortunately, you're the only recently active collaborator to this repo. Is there another collaborator you can suggest that I ask to review and land this PR? This bug is blocking us. Thanks! |
|
@wycats can you find someone to review this PR, please? |
|
@ErisDS do you have any time to review this one? |
|
Just hit this using handlebars for the first time as part of frctl/fractal for building a component-focused interface. Anything I can do to help get this out the door? My use case is similar to the original reporters: paragraph.hbs blockquote.hbs With the latter it basically wraps and passes it's |
|
@lawnsea this looks pretty good to me. Do you know whether this regresses performance for Handlebars templates that aren't using partials? I generally try to make |
|
@wycats I don't expect it to, no. The code path I modified should only be hit when resolving a partial named |
|
@wycats Here are the benchmark results:
master
|
|
The numbers, at first glance, seem to show a regression (arguments: 285 -> 328, but with ±100+). If you think there's no reason for that, it's probably just noise. Let me look a little more closely at the code to make sure nothing jumps out and then I'll merge. |
|
After thinking about it more, this look good. Good work 😄 |
|
Sorry it took so long to merge, and thanks so much for your contribution. |
|
@wycats Thanks! |
|
Thanks for getting this merged, can't wait to take advantage of it! Out of curiosity, I notice there hasn't been a tagged release since last November, should I just be using GitHub as the source or am I missing something? |
|
This popped up on my weekly checkin again, what is the status of how people should be installing handlebars? Is this not published to NPM anymore? |
The root cause of #1218 is that
invokePartialcreates a stack of data framesfor nested partial blocks, but
resolvePartialalways uses the value at top ofthe stack without "popping" it. The result is an infinite recursive loop, as
references to
@partial-blockin the partial at the top of the stack resolve toitself.
So, walk up the stack of data frames when evaluating. This is accomplished by
partial-blockproperty tonoopafter use and_parent['partial-block']ifpartial-blockisnoopFix #1218