-
Notifications
You must be signed in to change notification settings - Fork 191
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
[BUGFIX] Process in-element in compiler not parser #1086
Conversation
This fixes the issue described in DockYard/ember-maybe-in-element#25 (comment) tl;dr If an AST transform introduces the element, it doesn't go through the parser, so it's missing the required `guid` and `insertBefore` normalization. This commit moves the processing to a later stage to avoid this issue.
@@ -18,8 +18,6 @@ function isTrustedValue(value: any) { | |||
return value.escaped !== undefined && !value.escaped; | |||
} | |||
|
|||
export const THIS = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was not used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, it was dead code. Unless it’s some kind of public API but then since the entire codebase doesn’t do anything with it I don’t know what that would do either.
@@ -62,6 +66,7 @@ export default class TemplateCompiler implements Processor<InputOps> { | |||
} | |||
|
|||
startProgram([program]: [AST.Template]) { | |||
this.cursorCount = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully understand why we are resetting this counter for each program. I would have assumed that it was important that the guid
we used was unique across each template (or is it needing to be more globally unique?), but resetting in startProgram
here will lead to the same template reusing the same cursorCount (and therefore this.cursor()
not being unique across the template).
D'oh! I just realized (after digging more into the code) that startProgram
here is super misleading. It really should have been renamed to startTemplate
when we split the usages of Program
AST nodes into Template
and Block
.
I do still wonder what level of uniqueness we are looking for with the guid, but that is somewhat unrelated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhat ironically, the original code did suffer from the problem I described since it is operating on the Handlebars.Program
(which is definitely emitted for each branch of a block).
See this AST Explorer showing that multiple Program
nodes would have been seen in the HandlebarsNodeVisitors
and therefore the same cursor would be reused within a template like
{{#if foo}}
{{#in-element this.el}}{{/in-element}}
{{else}}
{{#in-element this.el}}{{/in-element}}
{{/if}}
Thank you @gitKrystan! |
Prior to this, an AST transform that adds an `in-element` usage would fail subtly. This was fixed in glimmerjs/glimmer-vm#1086.
Prior to this, an AST transform that adds an `in-element` usage would fail subtly. This was fixed in glimmerjs/glimmer-vm#1086.
Prior to this, an AST transform that adds an `in-element` usage would fail subtly. This was fixed in glimmerjs/glimmer-vm#1086.
Prior to this, an AST transform that adds an `in-element` usage would fail subtly. This was fixed in glimmerjs/glimmer-vm#1086. (cherry picked from commit 5a8c72a)
This fixes the issue described in DockYard/ember-maybe-in-element#25 (comment)
tl;dr If an AST transform introduces the element, it doesn't go through the parser, so it's missing the required
guid
andinsertBefore
normalization. This commit moves the processing to a later stage to avoid this issue.