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

Rewrite inline let to have lexical scope #12

Merged
merged 1 commit into from
Aug 30, 2016

Conversation

mmun
Copy link
Contributor

@mmun mmun commented Aug 29, 2016

EDIT: The syntax of {{let}} and {{#let}} has changed since this PR.

This PR reimplements inline let as an HTMLBars AST transform. There is no notion of inline let at runtime. It works by rewriting the let statements as nested blocks. For example,

<div>
  {{let a 1 b 2}}
  {{let a 11 b 12 c (add a b)}}
  {{a}} {{b}} {{c}}
  <div>
    {{let a 5}}
    {{a}} {{b}} {{c}}
  </div>
  {{let a (add a 10)}}
  {{a}} {{b}} {{c}}
</div>

is transformed to (excluding whitespace changes to make it easier to read):

<div>
  {{#let 1 2 as |a b|}}
    {{#let 11 12 (add a b) as |a b c|}}
      {{a}} {{b}} {{c}}
      <div>
        {{#let 5 as |a|}}
          {{a}} {{b}} {{c}}
        {{/let}}
      </div>
      {{#let (add a 10) as |a|}}
        {{a}} {{b}} {{c}}
      {{/let}}
    {{/let}}
  {{/let}}
</div>

Both templates generate the DOM

<div>
  11 12 3
  <div>
    5 12 3
  </div>
  21 12 3
</div>

@taras
Copy link
Member

taras commented Aug 29, 2016

@mmun thank you!

Ember 1.13 test is failing. In the current test suite, we're not testing inline let in 1.13.

Should we exclude the AST transform from for Ember 1.13 or is it possible to make it work with 1.13?

@mmun
Copy link
Contributor Author

mmun commented Aug 29, 2016

Should be possible. :)

EDIT: Actually, it would be a pain to get it working in 1.13 so I disable the transform below 2.0.0.

@mmun
Copy link
Contributor Author

mmun commented Aug 29, 2016

I've changed the example in the PR message to demonstrate that bindings are created in parallel rather than series (E.g. the semantics are like LISP's let rather than let*).

@Robdel12
Copy link
Collaborator

This is pretty awesome 🎉

@@ -17,22 +13,6 @@ export default {
options.template.yield(params);
}
Copy link
Member

Choose a reason for hiding this comment

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

For < 2.0 users, shouldn't there be an else with a warning that inline usage is not allowed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can do that.

@fivetanley
Copy link

is this glimmer 2 compatible?

@cowboyd
Copy link
Member

cowboyd commented Aug 30, 2016

An excellent question, is there a way that we can test it?

@Robdel12
Copy link
Collaborator

Robdel12 commented Aug 30, 2016

emberjs/ember.js#14156 looks like we can build vs canary?

@cowboyd
Copy link
Member

cowboyd commented Aug 30, 2016

So looks like it's going to require some work to get it working on Glimmer. We can track it with #14, but I don't think it should hold this up getting merged.

@cowboyd cowboyd merged commit 1f930e1 into thefrontside:master Aug 30, 2016
@cowboyd
Copy link
Member

cowboyd commented Aug 30, 2016

I can't begin to express how happy this makes me. A truly lexical inline form is almost bringing tears to my eyes y'all. Not even joking. Thanks @mmun and @rwjblue making this happen.

@taras
Copy link
Member

taras commented Aug 30, 2016

Absolutely, thanks guys so much for helping us make this work.

@mmun mmun deleted the lexical-inline-let branch August 30, 2016 16:32
@mmun
Copy link
Contributor Author

mmun commented Aug 30, 2016

@cowboyd I have a follow up PR soon that adds the warning.

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

Successfully merging this pull request may close these issues.

5 participants