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

Named Arguments Syntax ({{@foo}}) #276

Merged
merged 8 commits into from
Jan 3, 2018
Merged

Named Arguments Syntax ({{@foo}}) #276

merged 8 commits into from
Jan 3, 2018

Conversation

chancancode
Copy link
Member

@chancancode chancancode commented Dec 10, 2017

@mmun
Copy link
Member

mmun commented Dec 10, 2017

This bridges the gap nicely with the named block params described in #226.

For example, with this RFC you could have a component with a template like

The title: {{@title}}
The data: {{@data}}

There is an analogous example with named block params:

{{#x-modal as |@title @data|}}
  The title: {{@title}}
  The data: {{@data}}
{{/x-modal}}

@josemarluedke
Copy link

This is awesome. Good to see Glimmer.js ideas getting into Ember.

Would the component invocation be the same as it is today?

{{hello-world name="Godfrey"}}

Or would it be similar to what Glimmer.js has:

{{hello-world @name="Godfrey"}}

I feel the second one can help people see the relationship between of the arguments and it being used in the templates.

@les2
Copy link
Contributor

les2 commented Dec 10, 2017

wishful thinking that within the scope of this rfc:

  1. the guides are updated to cover this new usage

  2. in fact, all examples should use this syntax to encourage its use --- we don't need to different ways of accessing named props

  3. if possible, deprecation warnings for the old usage (would have to target 4.x as no new 3.x deprecations are currently allowed, right?)

  4. move the old behavior (automatically "reflecting" named arguments on the instance) into an addon; this allows users who want to move forward to delete this addon and fully take advantage of the performance gains; addon authors can conditionally include this addon when they detect older versions of ember. the legacy addon should be able to do the ast transform on addons using the new syntax so that they work with older ender versions.

i would really like to avoid a situation where people are writing code that isn't covered in the guides. imo, the guides should reflect what we should be doing / the best practices for a given ember version.

i would also like to ease the path for addon authors who want to take advantage of this while continuing to support older versions of ember. creating an addon that converts newer syntax to older api would allow this in a relatively painless way (similar to the other polyfills).

@rwjblue
Copy link
Member

rwjblue commented Dec 10, 2017

Would the component invocation be the same as it is today?

{{hello-world name="Godfrey"}}

Yes, it would use today’s invocation system.

@rwjblue
Copy link
Member

rwjblue commented Dec 10, 2017

wishful thinking that within the scope of this rfc:

the guides are updated to cover this new usage

Totally agreed.

in fact, all examples should use this syntax to encourage its use --- we don't need to different ways of accessing named props

Agreed.

if possible, deprecation warnings for the old usage (would have to target 4.x as no new 3.x deprecations are currently allowed, right?)

I don’t quite agree here. We should definitely plan to deprecate using a provided arg without the @ prefix, but we should have a few versions of overlap without the deprecation. This allows users to utilize the new syntax (and for us to adjust the guides / etc) before issuing deprecations.

move the old behavior (automatically "reflecting" named arguments on the instance) into an addon; this allows users who want to move forward to delete this addon and fully take advantage of the performance gains; addon authors can conditionally include this addon when they detect older versions of ember. the legacy addon should be able to do the ast transform on addons using the new syntax so that they work with older ender versions.

I agree generally speaking, but even if we did this the addon would have to be included by default (due to semver requirements). This isn’t a hard blocker, but it is a thing we need to be very careful about...

As an aside, the ambiguity that causes confusion for human readers is also a
problem for the compiler. While it is not the main goal of this proposal,
resolving this ambiguity also helps the rendering system. Currently, the
"runtime" tempalte compiler has to perform a helper lookup for every `{{name}}`

Choose a reason for hiding this comment

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

Spelling mistake: tempalte

Copy link
Contributor

Choose a reason for hiding this comment

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

Témpalte, por favor.

@mike-north
Copy link
Contributor

Often developers like to put "default values" for properties (including "arguments" that may be passed in by the caller) on their component prototype.

Component
class CourseWidget extends Component {
    isRequired: false // isRequired=false by default
}
<h1>{{name}}</h1>
{{#if isRequired}}
   <span class='required-indicator'>YOU MUST TAKE THIS COURSE</span>
{{/if}}
Top-Level Template
<!-- We can provide a value for the isRequired argument -->
{{course-widget name="Physics" isRequired=true}}
<!-- or not -->
{{course-widget name="French Literature"}}

Is the intent to continue supporting and encouraging this pattern?

Related: the React.Component#defaultProps approach to solving this is not terrible.

@sohara
Copy link

sohara commented Dec 11, 2017

Am I correct in assuming that positional params will be treated like named arguments in this proposal? I don't find any evidence of support for positional params in glimmer.js, so perhaps these won't be supported in Glimmer components in Ember?

@tomdale
Copy link
Member

tomdale commented Dec 11, 2017

@les2 I totally agree with you regarding documentation and only having one idiomatic way to do this. I think there's a bit of context missing from this RFC, though, that help shapes the right answer here. (To be upfront, we are trying a new strategy of focusing on small, very focused, very incremental RFCs. The downside is that the "big picture" plan is not as obvious, but hopefully the increased velocity is worth the tradeoff.)

One thing we've been discussing is adopting an system like Rust's epochs. The tension we're trying to resolve is how you keep development fast, iterative and lightweight while still presenting a cohesive library to outside observers.

With epochs, the idea would be to rapidly land a series of small, incremental changes related to improving some part of the system. In between those changes landing, the system may feel less coherent, or like there's multiple ways to do the same thing. Once a big enough set of related changes land that allow us to present a unified mental model, we declare a new "epoch" and use that time to create a snapshot of the programming model, update examples and guides, etc.

There's way more details here and I'm probably not doing it justice, and I want to make sure I don't turn this into a meta-RFC on epochs and derail @chancancode's RFC. But I did want to chime in that one reason we may want to hold off on doing a guide pass immediately is there are a set of related changes we want to do incrementally, and we'll save the learning team a lot of work (and produce a more cohesive whole) if we wait for all of those to land before doing a big push.

@wycats
Copy link
Member

wycats commented Dec 11, 2017

@mike-north

Is the intent to continue supporting and encouraging this pattern?
Related: the React.Component#defaultProps approach to solving this is not terrible.

Because this RFC does not remove the auto-reflection feature of Ember.Component at this time, this functionality will continue to work. Instead of accessing such an argument with {{@title}}, you would use {{title}}. In my opinion, this is actually a good thing, in the sense that it differentiates between arguments that are passed "as is" to the component from arguments that you can only understand by reading the JavaScript class. In fact, that differentiation is a key benefit of this (minimal) RFC, and I would like to avoid muddying the waters.

That said, there are several follow-on approaches we could use to help make the defaults use-case more ergonomic in the absence of auto-reflection. We would probably want to pick one of those approaches before actively encouraging people to avoid auto-reflection.

  1. We could implement something like React.Component#defaultProps using APIs currently available in the Glimmer VM. As I said, I think that I am uneasy about muddying the waters this way at this time, but I could imagine that with other clarifying improvements about the role of arguments (and in the context of a mental model without auto-reflection) that it might be more palatable.
  2. We could provide a way to define a signature for components, including the @names expected by a component, directly in the template. This would be analogous to function signatures in JavaScript (aside: I have long believed that allowing people to define in-template signatures would be a good opt-in feature). In that case, we could allow defaults to be expressed directly in the in-template signature, which would make them highly ergonomic.

I think that you have identified an important issue for us to work on as we continue to iterate towards the full feature-set of Glimmer components. I also believe that it's important for us to continue to land the full feature-set, so we don't end up "stuck" in a particular in-progress state for very long. In the meantime, this RFC is a much more incremental improvement that improves the clarity of templates, and does not disallow the existing default patterns (as long as the template explicitly differentiates these use-cases by using {{title}} instead of {{@title}}). In the interest of making progress, I think this is a good step.

@t-sauer
Copy link

t-sauer commented Dec 11, 2017

How are arguments accessed in the JavaScript file of the component? this.args.foo as in Glimmer.js?

@wycats
Copy link
Member

wycats commented Dec 11, 2017

How are arguments accessed in the JavaScript file of the component? this.args.foo as in Glimmer.js?

This RFC provides a way to access the arguments in templates as an incremental step, but it does not remove (or replace) auto-reflection as the way to access the values from JavaScript. For now, this.get('title') is still the mainline JavaScript API.

I think I would expect a follow-on RFC to propose this.args in JavaScript (as in Glimmer.js), as well as a way to opt out of auto-reflection.

@chancancode
Copy link
Member Author

@sohara it is a plausible feature (something like @0, @1, etc perhaps, and maybe another @arguments feature that gives you @arguments.positional.3 and @arguments.named.foo), but in the spirit of keeping the scope of s RFC minimal, we should explore those possibilities and benefits in another RFC.

Here are some reasons why it is not an obviously good/straightforward idea:

  • The Glimmer VM already supports @foo today but no support for @0 etc yet (which we can add, but it will just take some time).
  • The main usage of positional arguments today in curly components is to use the positional arguments mapping feature in the JS class (positionalParams: "allOfThem" or positionalParams: ["first", "second", "third"]). In either of those examples @allOfThem or @first/@second/@third would work as you expect in this proposal.
  • The angle bracket syntax doesn't support positional arguments intrinsically, so it's unclear whether we will need them in the long term.

The only thing worth considering in this RFC is perhaps to reserve @[number] and @arguments in the RFC so we can make those changes in the future. But we would probably want to make {{foo-bar arguments="..." 0="..." 1="..."}} in the invocation side also to make this truly work. I think the second part is out-of-scope here, but I think it's a good idea to discuss any possible keywords to reserve here (cc @wycats).

@acorncom acorncom mentioned this pull request Dec 11, 2017
@rwjblue
Copy link
Member

rwjblue commented Dec 11, 2017

I think the second part is out-of-scope here, but I think it's a good idea to discuss any possible keywords to reserve here

Agree. I think reserving @args and @arguments makes a ton of sense. I’m less sure about @<number> though.

@mmun
Copy link
Member

mmun commented Dec 11, 2017

@<number> doesn't parse today so it is de facto reserved :)

@chancancode
Copy link
Member Author

@sohara @mmun @rwjblue I added a section on reserved words (based on @ef4's suggestion)

@chancancode
Copy link
Member Author

I implemented the feature (behind a flag). You can try this on canary now: https://ember-twiddle.com/0c42585dc47fe38e118fffb9eec0714b?openFiles=templates.components.hello-world.hbs%2C

@scottmessinger
Copy link
Contributor

I love the goal of separating attributes from a component's properties, but I'm concerned about the use of the @ symbol. If we're teaching this, we have to teach two things:

  1. attributes is a concept
  2. We don't use attributes or an abbreviation like attrs but a symbol (@) to reference attrs in the template.

While the brevity of 1 symbol is appealing, it adds overhead to teaching and learning. Is there a technical reason to only use a symbol or is this for the sake of brevity? (My apologies if I missed that reason in the RFC). If it's only for the sake of brevity, my vote is to standardize on attrs. It's immediately evident that attrs is an object passed into the template while @ requires knowing of Ember. Also, searching the docs for "What does the @ sign mean" might not produce the right results as symbols are often treated differently by search engines.

@wycats
Copy link
Member

wycats commented Dec 12, 2017

@scottmessinger a few things.

First of all, I think "attributes" is pretty clearly not the correct concept (and it's the one we happen to use in Ember since 2.0 😱) because that concept has a related but not identical meaning in HTML.

I think something like "arguments" is more appropriate. I also think that teaching components as "functions that take arguments" is an extremely high-level thing to teach someone. I have often taught extremely confused people about this mental model and seen them very quickly get a lot of clarity. I don't think a model that tries to hide the concept of "arguments" from components is very effective, because the concept of "components and functions" is so powerful.

Similarly, I don't think that something like {{attrs.name}} or {{args.name}} is actually much of a simplification. It is precisely the fact that those solutions attempt to repurpose syntax that is, in my opinion, the problem.

It causes people to believe that {{args.name}} is the same thing as {{this.args.name}}, which is almost true, but is not actually true, and creates a much more complex mental model for arguments than is actually necessary. In particular, "passing an argument" needs to be understood as "passing a bucket of values that gets installed on a component object under the name args, and is then available as {{args}} in the template because it is on the component object.

I don't prefer this explanation both because it's more complicated, and because it doesn't even provide a very good rationale for template-only components, which I am motivated to elevate in importance.

TL;DR: I think adding a new concept of "arguments" is actually simplifying and clarifying, and helps to address some inconsistencies between reality and the ways people tend to think about the current API.

@mehulkar
Copy link
Contributor

mehulkar commented Dec 13, 2017

2 cents:

It would be nice if {{@foo}} was considered syntactic sugar for a more verbose helper such as {{get-arg "foo"}}. I think @ is not enough to indicate that a thing is different, because there's no reason for me, as a beginner, to believe that {{my-component @foo='bar'}} isn't possible. (At the very least, it would be nice to explicitly throw a nice error message if a developer tries to use this syntax, and that should be part of this RFC!).

Secondly, as @mike-north also suggests, I've always used default / fallback properties (especially for defining noop functions for when closure actions aren't passed in, so that code doesn't fail with undefined is not a function). @wycats's response to this issue indicates some drift between {{@title}} and {{title}}, which seems like a yellow/red flag to me, since using the fallback mechanism as I do now, will introduce ambiguity about where the "auto-reflected" property is coming from.

On that second note, I'm actually not clear on why it is important to differentiate between component defined properties and named args, since the pattern for "overriding" properties on objects already exists, even in Ember.Object.extend({foo: 'bar'}).create({foo: "no, this bar"}). I wonder if the future of this will necessarily involve not being able to define any properties on components, and only setting state in init or other lifecycle hooks? I think that could be a very good step forward, at least in terms of developer UX!

The first problem with this approach is that the `{{name}}` syntax is highly
ambigious, as it could be referring to a local variable (block param), a
helper or a named argument from the caller (which actually works by accessing
auto-reflected `{{this.name}}`) or a property on the component class (such as
Copy link
Contributor

Choose a reason for hiding this comment

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

I had no idea! I want to start using this yesterday.

Copy link
Contributor

@locks locks Dec 13, 2017

Choose a reason for hiding this comment

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

this.name? You can.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that I think about it, I vaguely remember that {{this.name}} was the right way to use properties in pre 1.0 or 1.x. Or something about {{this.name}} would not be proxied to the underlying model of the controller and {{name}} would be (or vice versa). But that is a side topic, don't want to take over this rfc 😄

@sandstrom
Copy link
Contributor

sandstrom commented Dec 13, 2017

My only comment is that if it makes sense, perhaps a few more words could be reserved?

Something like @internal, @debug, @prop. But I have no concrete idea for usage here, it's just that it's easier to reserve now than claw something back later.

@chancancode
Copy link
Member Author

chancancode commented Dec 13, 2017

One big reason is Glimmer Components will not have the auto-reflection semantics: you can get a glimpse of this now by trying this with #278 together on canary: https://ember-twiddle.com/0c42585dc47fe38e118fffb9eec0714b?openFiles=templates.components.hello-world.hbs%2C

(Updated/fixed link)

@locks
Copy link
Contributor

locks commented Dec 13, 2017

@mehulkar

(At the very least, it would be nice to explicitly throw a nice error message if a developer tries to use this syntax, and that should be part of this RFC!).

{{my-component @foo='bar'}} is already not possible in current Ember, so there'd be no change there!

@mehulkar
Copy link
Contributor

@locks i'm not surprised that it isn't possible right now (and that's great!), but my point is that as a beginner, I have no reason to believe that a string starting with @ is any different than a string that does not. Right now, it looks like using {{my-component @foo='bar'}} throws a generic error. I just think that will be really confusing and would at least want the guides to confirm that only [a-zA-Z] is supported in named args (or whatever the rule is).

@chancancode
Copy link
Member Author

chancancode commented Dec 16, 2017

This RFC was nominated and approved for Final Comment Period (FCP) yesterday, since most comments have already been replied to. The FCP with last for a week. If there are any substantial new arguments that are brought up during this time, the FCP will be restarted (or aborted). If you haven't reviewed the proposal yet, now would be a great time to do so.

If I may, here is a summary of the main objections:

  1. {{@foo}} does not support the full set of use cases that are possible with auto-reflected properties today.

    Auto-relection is still supported on Ember.Component and will not receive a deprecation warning any time soon. This may introduce some temporary coherence issues, and for this reason (as mentioned in the RFC and @tomdale's comment), we might not want to update the guides to recommend using the new syntax right away.

    However, as it unlocks other features (such as the template-only component RFC and Glimmer Components which does not rely on property auto-reflection), we think there is no need to halt progress because of that.

  2. Sigils are hard to teach/Google

    This is mainly covered in @wycats' reply, but @tomdale would like to take another stab and elaborate on the philosophy there.

So, I will wait for @tomdale to comment and let him initiate the official Final Comment Period.

Meanwhile, since the feature have landed in canary behind a flag, I encourage you to give it a spin and get a feel for it, either using this Ember Twiddle or by using a canary build with the ember-glimmer-named-arguments feature flag set to true.

@tomdale
Copy link
Member

tomdale commented Dec 20, 2017

Regarding sigils vs. reserved identifiers, I'm sympathetic to the arguments people have made against @ on the grounds of googlability, readability, etc. It's tricky to get right because the conservative application of sigils can both clean up code and reinforce mental models, but go just a little too far and even simple code can start feeling overwhelming (e.g. Perl or early versions of Rust).

Our design philosophy is that you get a "sigil budget" that you must spend very carefully. As @wycats mentioned above, I think having the concept of component arguments with first class syntax (i.e. a sigil) to access them is a good thing; the syntax helps reinforce the concept. I'm also persuaded by his argument that not distinguishing arguments via @ will muddy the mental model waters. If {{foo}} always accesses a property on the backing component, except for some non-obvious special cases like {{args}} (made all the more confusing by the existence of the actual args property!), that is worse than the explicit @ prefix which at least tells you something out of the ordinary is happening.

Ultimately, I think we are safe to FCP this for the following reasons:

  1. Experience with this syntax in Glimmer.js apps shows us that @names are at minimum non-fatal to understanding.
  2. Better language server support will make nudging developers towards the appropriate syntax much easier, if we provide an autocomplete of available symbols in scope, including the @ prefix as appropriate.
  3. Should we later decide that we don't want to spend our sigil budget on component arguments, reserving args means we can deprecate @name and codemod our way to args.name in the future. In other words, this design doesn't foreclose on the alternative design should we run into problems in practice.

@scottmessinger
Copy link
Contributor

@wycats Thanks for the response!

@wycats: I think adding a new concept of "arguments" is actually simplifying and clarifying, and helps to address some inconsistencies between reality and the ways people tend to think about the current API.

I completely agree with this. "arguments" is more appropriate than "attributes" and "this.args.name" or "args.name" isn't a simplification as it's almost but not exactly right.

If "arguments" is the powerful concept and the sigil @ is really helpful in distinguishing args vs props, why not put all named arguments under @arguments as in @arguments.name? Since accessing arguments is so common and seems to work with the glimmer community, we could keep the @name syntax as a shorthand. This makes explaining the @name form quite simple -- you can drop the arguments. as it's such a common pattern.

@mehulkar: "It would be nice if {{@foo}} was considered syntactic sugar for a more verbose helper such as {{get-arg "foo"}}".

This suggestion builds on @mehulkar's idea.

@mehulkar: I think @ is not enough to indicate that a thing is different, because there's no reason for me, as a beginner, to believe that {{my-component @foo='bar'}} isn't possible.

FWIW, actually quite like the idea of using the @name= syntax for creating components -- if we're referencing the arguments as @name, why not take out the confusion and declare them by using @name=? I know that's out of scope for this RFC, but seems worthwhile to consider if we think it could help clarify the diff between arguments and properties.

@mike-north: Often developers like to put "default values" for properties (including "arguments" that may be passed in by the caller) on their component prototype.

@wycats: We could implement something like React.Component#defaultProps using APIs currently available in the Glimmer VM

One advantage to using the word "arguments" is we can create something like defaultArguments or getDefaultArguments and they should make quite a bit of sense to devs without much explanation. I think React's benefited significantly from a similar simplicity: To access a prop in React, you have to use the word "prop" as in this.props. To add default props, you use the sensible named, getDefaultProps.

Another advantage to using @arguments is we can adopt the splat style common in React. E.g. {{my-component ...@arguments}}

TL;DR: If "arguments" is a powerful concept and since the sigil @ does an excellent job separating arguments from properties, let's introduce the @arguments hash for named arguments and allow @arguments.name to be shortened to @name.

@wycats
Copy link
Member

wycats commented Dec 23, 2017

@scottmessinger thanks for the thoughtful reply.

I want to reiterate that I think the best way to understand Ember components is as functions that you're calling with arguments.

So when you say: {{article-body title=name}}, it is the equivalent of calling a function named article-body with the named argument title with a value of name. I think it's critical that this is how we teach it and how people understand it.

To put it directly before diving into some more details, I don't think it would help JavaScript if people understood arguments first, and then learned that the first parameter in a function "desugars" into arguments[0].

Some details follow.

Components Have No Signatures (Yet)

Unlike functions in most languages, today's components have no "function signature".

You could imagine that a component with this body:

<h1>{{@title}}</h1>

Actually looks like this behind the scenes:

{{#def-component as |@title|}}
  <h1>{{@title}}</h1>
{{/def}}

where def-component is an explicit way of defining a component in the Glimmer programming language.

This is more or less how Glimmer works internally (the parser finds all of the @names in a component template and creates a synthetic signature that looks a lot like the above).

Another way to put it is that when you use @names in a template, they become parameters.

I personally don't like the fact that there's no explicit way to write this signature, and really want to be able to (for this and many other reasons). In the meantime, it's best to think of components as automatically taking any @names used inside of its template as arguments.

Glimmer Components Need Named Arguments

The reason Glimmer components mostly take named arguments rather than positional arguments (as JavaScript functions do) is that they are intended to be embeddable inside of markup that looks like HTML:

<ArticleBody @title={{name}} />

This means that our "arguments" are almost always named arguments in practice.

Because of the lack of an explicit signature in Glimmer components, we use a sigil to distinguish between named arguments and other names in scope (which include positional arguments from APIs like {{#each}} and lookups on the current JavaScript component).

Let's Not Couple All of the Improvements Together

We've occasionally considered a large, sweeping overhaul of the system: add mandatory signatures to components, eliminate {{foo}} as a shorthand for {{this.foo}}, eliminate the old {{foo-bar}} invocation syntax (with its foo=bar named argument syntax) in favor of <FooBar> so that @foo= pairs nicely with {{@foo}} inside of component templates.

But increasingly, this made the entire package impossible to ship incrementally, both in terms of the total design and implementation effort, but more importantly in terms of getting the community to incrementally adopt and provide feedback on the new features.

I personally believe that to regain some velocity on this package of features, it's important for us to find ways to ship parts of the feature set of Glimmer components incrementally but quickly. This may mean that in the interim, some of the landed features can only be understood in terms of a delta from the previous features, but I don't think we should allow that to stop us from landing features as we are able to implement them.

That said, I do think it's critically important for us to have the rough strokes of the full feature-set up front, and for us to be prepared to land an atomic update to the mainline guides that reflects the programming model of all of the features in the near term. This is a near-term compromise that allows us to make reasonable progress on landing long-desired-but-delayed parts of the Glimmer programming model in shipping versions of Ember.

I hope that made sense 😄

@chancancode
Copy link
Member Author

Thanks everyone for your participation! Since no major issues were found during the FCP, we will now close and merge this proposal.

@chancancode chancancode merged commit e3bf65a into master Jan 3, 2018
@chancancode chancancode deleted the named-args branch January 3, 2018 18:06
@wycats wycats mentioned this pull request Jan 7, 2018
@Kerrick
Copy link

Kerrick commented Jan 8, 2018

@chancancode The "Rendered" link in OP 404s. Can you please update it to the merged proposal?

@mmun
Copy link
Member

mmun commented Jan 8, 2018

@Kerrick Thanks. Fixed it.

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.