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

[BUGFIX] Process in-element in compiler not parser #1086

Merged
merged 2 commits into from
May 8, 2020

Conversation

gitKrystan
Copy link
Contributor

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.

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;
Copy link
Member

Choose a reason for hiding this comment

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

This was not used?

Copy link
Contributor

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;
Copy link
Member

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.

Copy link
Member

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}}

@rwjblue rwjblue added the bug label May 8, 2020
@rwjblue rwjblue merged commit 54b9e66 into glimmerjs:master May 8, 2020
@rwjblue
Copy link
Member

rwjblue commented May 8, 2020

Thank you @gitKrystan!

rwjblue added a commit to emberjs/ember.js that referenced this pull request May 8, 2020
Prior to this, an AST transform that adds an `in-element` usage would
fail subtly. This was fixed in
glimmerjs/glimmer-vm#1086.
rwjblue added a commit to emberjs/ember.js that referenced this pull request May 8, 2020
Prior to this, an AST transform that adds an `in-element` usage would
fail subtly. This was fixed in
glimmerjs/glimmer-vm#1086.
rwjblue added a commit to emberjs/ember.js that referenced this pull request May 8, 2020
Prior to this, an AST transform that adds an `in-element` usage would
fail subtly. This was fixed in
glimmerjs/glimmer-vm#1086.
@chancancode chancancode deleted the process-in-element branch May 8, 2020 17:43
kategengler pushed a commit to emberjs/ember.js that referenced this pull request May 12, 2020
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants