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

Block let template helper #286

Merged
merged 5 commits into from
Jan 18, 2018
Merged

Block let template helper #286

merged 5 commits into from
Jan 18, 2018

Conversation

locks
Copy link
Contributor

@locks locks commented Dec 21, 2017

Rendered

This RFC is intended as a subset of #200.

Changelog

v2

  • Refocused "Motivation" on the benefits of the block let
  • Move "Inline Form" to alternatives and expand
  • Move with deprecation to "Future Work"
  • Moved if-let, let* to "Future Work"
  • Introduce "Using Components" to "Alternatives"

@kellyselden
Copy link
Member

Another alternative is to remove the blockers from making ember-let use public api only. The two blockers I can think of are one word components, and yielding N number of items. If modularizing and extracting core is the way forward, seems like that might be a equivalent proposal.

@locks
Copy link
Contributor Author

locks commented Dec 21, 2017

@kellyselden that is indeed another possibility, albeit a harder sell on account of "yielding N number of items" meaning introducing a way to splat arguments (were you thinking something else?), which is its own can of worms ;P

As for removing the "needs a dash" constraint on component names, it has been brought up before but I'm not sure what the general sentiment is. Internally, I believe the presence of a dash in the name is part of the heuristic to disambiguate between locals, helpers, and components.

To touch on the modularity question, for some time now we had the idea of making a "standard library" addon with template helpers that could be shared amongst Emberjs and Glimmerjs.
[Un]fortunately, due to versioning drift and the current codebases, it isn't trivial either to extract the helpers, or to share the hypothetical stdlib between projects.
I'm mentioning this as it might provide insight into my viewpoint.

Copy link

@marcoow marcoow left a comment

Choose a reason for hiding this comment

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

I think the block form of the let helper is effectively the same thing as extracting that block into another component:

{{#let
  firstName=(capizalize person.firstName)
  lastName=(capitalize person.lastName)
}}

  Account Details:
  First Name: {{firstName}}
  last Name: {{lastName}}
{{/let}}

becomes

{{person-tile firstName=(capizalize person.firstName) lastName=(capizalize person.lastName)}}

where the person-tile's template looks like this:

Account Details:
First Name: {{firstName}}
last Name: {{lastName}}

Of course the capitalized strings could also be defined in JS land (e.g. using @kellyselden's ember-awesome-macros) when passing the full person into the component:

{{person-tile person=person}}
firstName: string.capitalize('person.firstName'),
lastName: string.capitalize('person.lastName')

Extracting new components for cases like this obviously leads to more code and more components but also doesn't introduce a new concept…

@cafreeman
Copy link
Contributor

IMO this isn't so much a new concept as it is a less surprising version of an existing concept (the with helper). I don't disagree that there are cases where let usage could be achieved by extracting to a component, but it's not always the case that there's a clear boundary for extracting to a component.

In addition, being able to bind values in the same template where they're being used is often more straightforward when you're essentially just using it to set up a computed value without declaring it on the JS file (which would be crucial in template-only components, as @locks mentioned).

@knownasilya
Copy link
Contributor

I'm also of like mind that this is the better version of with, which I use much less due to it's "don't render if falsy" behavior, but would love to use more.

@marcoow
Copy link

marcoow commented Dec 21, 2017

IMO this isn't so much a new concept as it is a less surprising version of an existing concept (the with helper).

I think what I'm trying to say is that this is the same as using a nested (template-only) component as both that and this let helper open up a new scope that allows renaming values. The only difference between both solutions that I see is really that with components you're forced to split the code in 2 templates while with let you can keep everything in one file. I'm not sure whether there are other benefits of let that I'm missing (or whether 1 vs. 2 files is considered enough benefit for adding let).

- Refocused "Motivation" on the benefits of the block `let`
- Move "Inline Form" to alternatives and expand
- Move `with` deprecation to "Future Work"
- Moved `if-let`, `let*` to "Future Work"
- Introduce "Using Components" to "Alternatives"
@locks
Copy link
Contributor Author

locks commented Dec 21, 2017

I have updated the RFC with some of the feedback in the thread.

I hope to have addressed your concerns!

@wycats
Copy link
Member

wycats commented Jan 4, 2018

I'm moving this into Final Comment Period.

@btecu
Copy link

btecu commented Jan 5, 2018

Anyone can provide a convincing example of where this would be useful?
I have never been able to come up with a case where the existing with helper was helpful or it significantly improved the developer experience.

Current RFC examples are bad (nonsense) or very unconvincing:
Why would you even do {{concat (capitalize person.firstName) ' ' (capitalize person.lastName)}} as opposed to {{capitalize person.firstName}} {{capitalize person.lastName}}?

Because you have to know to capitalize every time you want to display a name, errors might be introduced if we forget to do it when adding the name somewhere else in the template.

The whole argument of forgetting to use {{capitalize}} seems very weak.
In addition, this could be easily solved by having a computed on the model:
return `${firstName} ${lastName}`.toUpperCase();.
For that matter, one could just a css class to make the full name text upper case.

It appears that the let helper would allow variable declaration in template, which seems like a bad idea.

@wycats
Copy link
Member

wycats commented Jan 5, 2018

@btecu in general, the use case for #with (and now #let) is to take longer expressions and turn them into shorter variables that can be used inside of some block.

There are cases where bigger expressions come up naturally in Glimmer templates, most notably cases where you're yielding larger values like (component ... rest of the call ...). These expressions exist already, and #let allows you to avoid creating even larger expressions that come up when you are forced to inline all parts of the expression together.

Glimmer's templating language is a very small programming language. It has no mutable variables (really no mutation of any kind) and all of its constructs are pure. But Glimmer has never eschewed the ability to declare variables, and in fact the entire point of the as |foo| syntax is to make it possible to introduce variables into the Glimmer program.

@simonihmig
Copy link
Contributor

Anyone can provide a convincing example of where this would be useful?
I have never been able to come up with a case where the existing with helper was helpful or it significantly improved the developer experience.

@btecu Here is a real world example where I used with: https://github.com/kaliber5/ember-bootstrap/blob/master/addon/templates/components/common/bs-form/element.hbs#L28-L78

The component setup is quite verbose, and is used in the following if as well as the else block. So you certainly don't want to duplicate all of this twice. (also error prone when updating arguments)

@Panman82
Copy link

Panman82 commented Jan 5, 2018

Anyone can provide a convincing example of where this would be useful?
I have never been able to come up with a case where the existing with helper was helpful or it significantly improved the developer experience.

@btecu I have a use case (private repo) where I {{#each}} through an array and {{concat}} the same things ten times. I do use {{#with}} in this particular instance because I know I won't run into the falsy issue, but knowing that could be a problem, {{let}} would be nice..

@knownasilya
Copy link
Contributor

knownasilya commented Jan 5, 2018

I was searching through one of my code bases and found a {{#with}} block to paste as an example, but I ended up finding a bug:

{{#with (count-allowed group.themes 'view theme in dataset') as |allowedThemes|}}

Since the count could be 0 the contents of the block would be hidden, which I didn't want. So I just fixed a bug which wouldn't have existed with {{#let}}.

@wycats
Copy link
Member

wycats commented Jan 5, 2018

@knownasilya funny story, Glimmer treats only false, null and undefined as falsy, so this would actually not have had the wrong behavior, but the fact that you thought it would kind of underscores the point.

#let is basically like #with, but without having to think about whether the value you're using happens to be falsy.

@locks
Copy link
Contributor Author

locks commented Jan 8, 2018

Hey everyone! I did a Godfrey and a feature flag for this RFC is now in canary. You can try let out: https://ember-twiddle.com/3ed48c13bdbd2404b0185b1febab2aaa.

Using the `let` helper, this could be done like so:

```handlebars
{{#let (capizalize person.firstName) (capitalize person.lastName)
Copy link

Choose a reason for hiding this comment

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

capitalize is misspelled here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you :)

Copy link

Choose a reason for hiding this comment

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

No worries. There's actually another misspelling further down in the document too on L125.

@sdhull
Copy link

sdhull commented Jan 8, 2018

FWIW I recently tried to use #with to shorten an expression I was using multiple times in a template, and yes I was briefly befuddled when my template didn't render (the value would often be null).

Sorry I don't recall specifically what it was but I eventually resorted to an additional tiny component. I wasn't thrilled about it.

@wycats
Copy link
Member

wycats commented Jan 8, 2018

@sdhull I've had that experience tons of times too.

@LevelbossMike
Copy link

LevelbossMike commented Jan 10, 2018

I agree with @btecu and @marcoow. This isn't really something that can't be solved with existing concepts (e.g. a computed or a component). Though I understand why one would want to do something like this (it seems like the "easy" thing to do in the described scenarios) I don't think the usage of helpers that encourage to put logic into templates should be encouraged by the framework via an "official" helper. especially for simple cases that would best be solved by a very simple computed.

@simonihmig
Copy link
Contributor

This isn't really something that can't be solved with existing concepts (e.g. a computed or a component).

@LevelbossMike not so sure about this in every case. When you look at the example I posted in #286 (comment), how could that be done without with/ let? AFAIK you cannot setup a component in a CP, like I do it there with the component helper. And wrapping that in just another component doesn't make sense either...

I think the block form of the let helper is effectively the same thing as extracting that block into another component

@marcoow I think this is not exactly true, because as far as I understand it a let block would extend the current scope, while a component creates a completely new scope. Given your example in #286 (review), and when we also have bindings a - e that we need in our template, the component equivalent would have to look like:

{{person-tile a=a b=b c=c d=d e=e firstName=(capizalize person.firstName) lastName=(capizalize person.lastName)}}

Of course that get's even worse with more bindings...

@marcoow
Copy link

marcoow commented Jan 10, 2018

@simonihmig: yes, that's correct - a let block extends the current scope while a component creates a new isolated scope. I'm not sure how relevant this difference is in practice though.

To be clear, I'm not totally opposed to this - I'm just not sure it's hugely valuable. To be fair, I also didn't even know the with helper existed though ;)

@LevelbossMike
Copy link

@simonihmig as you pointed out that's a use-case for the with-helper. I'm not convinced that let adds any value on top of that that cannot be solved (and imo should be solved) via a computed in simple cases or component for more complex scenarios.

@locks
Copy link
Contributor Author

locks commented Jan 10, 2018

@LevelbossMike

I agree with @btecu and @marcoow. This isn't really something that can't be solved with existing concepts (e.g. a computed or a component).

As mentioned in the RFC, introducing bindings to the template is an existing concept, as that is what with does, with the "downside" of the conditional rendering of the block.

I don't think the usage of helpers that encourage to put logic into templates should be encouraged by the framework via an "official" helper.

let does not contain any logic. It doesn't even conditionally render the block like with ;P

especially for simple cases that would best be solved by a very simple computed.

Wouldn't this be an argument for let? Having to create a new file, whether component or helper, just for a simple computed property, polluting the global namespace until module unification lands, versus having a let block. I'd also like to suggest that you keep in mind the named args and the template-only components RFCS, especially the latter one.

Stealing an example from the Twiddle I linked above, I use let to pre-build a component with some arguments already passed in.

{{#let
    (component 'my-post' title=(concat post.title ' | The Ember.js Blog') content=post.content)
    (hash
    	theme="dark"
        enableComments=true
    )
    (hash
      theme="responsive"
      enableComments=false
    )
    as |prefilledPost darkTheme mobile|
}}
  {{component prefilledPost options=darkTheme}}
  {{component prefilledPost options=mobile}}
{{/let}}

You could do this with a component, but it would still be done in that component's template and you lose the colocation.

In general, this proposal does not change the immutable, declarative programming model of Ember templates.

@simonihmig
Copy link
Contributor

Two more thoughts regarding the latest points being discussed:

  1. you cannot use a CP (as an alternative to let) in a {{#each}}, when the computation depends on the current item

  2. using a component feels a bit like misusing the concept (of a component) for the wrong purpose. But maybe that's just me. The fact that it creates a new scope is more an implementation detail IMHO, the purpose is more like encapsulating functionality, reusability etc. "Separation of Concerns", "Single responsibility", any other buzzword? 😝
    Using it only for modifying the scope, while not really caring about encapsulation or reusability feels wrong to me.

The same thoughts could be applied to JS itself: you could change the scope by calling another function, passing the new scope as args (similar to a component invocation), thus eliminating the need for a let assignment. But that's certainly not the problem the concept of a function tries to solve, and not something any of us would want to do...

tl;dr I am pro let 😉

@wycats
Copy link
Member

wycats commented Jan 10, 2018

you cannot use a CP (as an alternative to let) in a {{#each}}, when the computation depends on the current item

For what it's worth, this was the original motivation for itemController, but we eventually came to believe that trying to force everything into a computed property causes more harm than good to the programming model, and that people should use helpers in many of these cases. Which puts a little more pressure on features like #let.

@wycats
Copy link
Member

wycats commented Jan 10, 2018

The same thoughts could be applied to JS itself: you could change the scope by calling another function, passing the new scope as args (similar to a component invocation), thus eliminating the need for a let assignment. But that's certainly not the problem the concept of a function tries to solve, and not something any of us would want to do...

Right! Before ES6 and block-scoped let, it actually was very common to use an IIFE (immediately-invoked-function-expression) to get a new var scope. I think everyone's very happy to see that pattern go bye-bye in favor of let scoping in JS.

I also really appreciate using analogies to other programming languages when designing and using Ember's templates. It's the right way to think about it, and goes a long way towards keeping the overall model small and simple.

@locks
Copy link
Contributor Author

locks commented Jan 18, 2018

Hey everyone, this RFC was discussed at the last Core meeting and we decided to give it the green light, as the concerns raised during the FCP are addressed in the proposal.
Thank you for participating, see you on the next one :)

@locks locks merged commit 395a897 into emberjs:master Jan 18, 2018
@locks locks deleted the block-let-helper branch January 18, 2018 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.