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

Glimmer Components #416

Merged
merged 4 commits into from
Jan 22, 2019
Merged

Glimmer Components #416

merged 4 commits into from
Jan 22, 2019

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Dec 14, 2018

rendered

Huge thanks to @tomdale, @rwjblue, @krisselden, @mike-north, @stefanpenner, @chadhietala, @MelSumner, and everyone else who helped with feedback and design in this RFC! This has been a long time coming, and represents a massive community effort that I have been very glad to be a part of!

@lennyburdette
Copy link

lennyburdette commented Dec 14, 2018

This is fantastic. Nice write-up!

I think that mentioning event handlers is warranted. While <button onclick={{action ...}}> is probably sufficient in most cases, losing the event methods on the component class has at least two drawbacks.

  1. The classic component event methods hooked into Ember's event delegation system and therefore are potentially more efficient.

    {{#each (times 100000)}} <button onclick={{action ...}}></button> {{/each}} assigns 100k event attributes in the DOM.

    On the other hand, {{#each (times 100000)}} <XButton @click={{action ...}} /> {{/each}} does not assign any event attributes, instead using the "click" event handler on added to the application's root element.

    I don't know the memory and performance implications of DOM event attributes in modern browsers, but AFAIK event delegation still has performance wins.

  2. The focusIn and focusOut class component event methods don't have DOM attribute equivalents! This is important because the onfocus and onblur DOM attributes don't bubble. Here's a twiddle demonstrating the issue. (It doesn't look like react has solved this either.)

I'm guessing that element modifiers are the way forward and I would be perfectly happy with that. Could you add something to the RFC clarifying this? Maybe just a section in the "How we teach this".

Edit: I forgot to mention the existing {{action}} element modifier because it doesn’t exist in Glimmer.js. It does use event delegation. But it’s a PITA because it’s the one method of assigning event handlers that doesn’t give you access to the underlying DOM event.

@pzuraq
Copy link
Contributor Author

pzuraq commented Dec 14, 2018

Great points! I think we should definitely add a section mentioning the reasons why we are not including event handler hooks on the components, and we can also discuss the possibilities for solving those problems. Addressing them individually:

  1. There may be some wins there, particularly in the laziness of "pulling" on the event handler function and binding it. Either way though, I believe an event handler function would have to be made somewhere per item, and redirected to at some point. I'm not sure about this at all, but I would imagine that a global jquery style event handler (how handlers on classic components appear to be routed, even post-Jquery) would be somewhat more expensive than letting the browser do the handling using standard event dispatching. Really unsure about that though, would have to benchmark to find out I think 😄

  2. I definitely think this is something we would need to add using element modifiers somehow, it seems like addEventListener has the behavior we're after.

I do think outlining specific APIs is out of scope for this RFC, but we should definitely mention current alternatives for users who are used to classic components in "How we teach this", specifically use either {{action}} or onclick= style event handlers.

@service time;

// Use the values of args in an initializer
fullName = `${this.args.firstName} ${this.args.lastName}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is really neat. Just to clarify, this would not be tracked, right? So, if the args change, fullName wouldn't update, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, for two reasons:

  1. The property is not marked as tracked, so it will not trigger a rerender even if it is changed (unless you use set)

  2. The property is not a getter, so it doesn't know how to recalculate even if it knows it's dependent on the values of the args. In that sense, this would just be a convenience for initial setup.

@rwjblue rwjblue added the T-framework RFCs that impact the ember.js library label Dec 15, 2018
API of a component, but this also has the potential for misuse and enables
antipatterns.

Glimmer components assign their arguments to the `args` property on their
Copy link
Member

Choose a reason for hiding this comment

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

Can you update this section to add the "new way" to author the classic syntax just above?

not make major changes without first getting community input, and will be
considered part of the public API of Ember.

## Prior Art
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps list react here as well?

@mike-north
Copy link
Contributor

Is there any concept of positional params in the angled/glimmer component world?

<form onSubmit={{action foo}}>
  <CustomInput @firstName> First Name </CustomInput>
  <CustomInput @lastName > Last Name  </CustomInput>
</form>

@pzuraq
Copy link
Contributor Author

pzuraq commented Dec 18, 2018

In the Angle Bracket Invocation RFC positional parameters were decided against for angle bracket syntax. I believe the reason this came to be was the difficulty parsing positional parameters vs. attributes, for instance:

<option selected></option>

It's difficult to tell if selected is meant to be a positional parameter or an attribute.

However, positional parameters are still legal in curly syntax and will continue to be, which was part of the reason for having both invocation styles. Glimmer components do not support positional parameters in the current proposal, and I believe the use cases are rare enough that a custom component manager (that does things like enforce that they are defined, and asserts against users using angle bracket invocation for them) would be a good alternative.

This should definitely be mentioned in the RFC either way, thanks for bringing it up!

@rwjblue
Copy link
Member

rwjblue commented Dec 18, 2018

Is there any concept of positional params in the angled/glimmer component world?

Just to reiterate @pzuraq's point, we need to be very careful to not infer that the invocation style is related to the base class used (mostly because that was true in past revisions of these features)...

@michaelrkn
Copy link

What's the reason for having a new class for Glimmer components? Why not add these features incrementally to the existing components API?:

  • Add the .args property and deprecate accessing properties otherwise.
  • Let users opt into outer HTML semantics, either through ember feature:enable template-only-glimmer-components or maybe even on a component-by-component basis, eg:
export default Component.extend({
  outerHtmlSemantics: true
});
  • Deprecate the properties and hooks that aren't part of Glimmer components.
  • Users can already use ES6 classes as of Ember 3.6.

It seems like this would make it easier for existing users to move to this new approach to components, and easier for new users to not have two component classes to consider. (And Ember 4.0 can drop the deprecated the features, and what's left is Glimmer components.)

@pzuraq
Copy link
Contributor Author

pzuraq commented Dec 22, 2018

@michaelrkn that's a great point! This has been considered over time but I think has been seen as undesirable and problematic for a number of reasons. We should definitely add a section addressing this in Alternatives, but to answer directly:

It's still a single massive shift

The biggest one to me is that we have such a large shift, that no matter what we effectively end up with two "classes". As you pointed out, outer HTML semantics, which are perhaps the biggest shift for Glimmer Components, would still require users to opt-in via a feature flag or per-component flag. Being able to transition one component at a time is important, so a per-component flag would definitely be necessary.

This flag would essentially disable a whole bunch of hooks and properties (all of the event hooks, tagName, classNames, element, etc), which would basically make the component behave like a completely different class. Instead of actually having two classes though, we would be doing this dynamically. Two class APIs, if you will.

There is no middle state for most of these features. You can't start using outer HTML and then get rid of tagName and classNames and event hooks one PR at a time. You'll have to rewrite your component altogether as soon as you turn it on.

Typings and performance

Typescript is starting to become more commonly used in Ember apps, and even users who don't use it are able to benefit from typings. It's a very nice DX to be able to import the component class and immediately see what properties are part of its public API. This is possible today with Ember components, but there's a lot of noise (over 50 props and methods) so this makes it less helpful. Combine this with the outer-html feature flag, where you are effectively changing the public API based on a flag, and it becomes even less helpful.

Additionally, Glimmer components having a much smaller API will be immediately better for performance, as will them not inheriting from EmberObject. This will help both new apps and larger apps that are performance sensitive as soon as they begin to transition to Glimmer components, instead of waiting another year or two for the extra hooks to be removed and even longer for the full removal of EmberObject from classic components.

Confusing conventions

Part of the issue with attempting to add these APIs incrementally is it may make it much less clear what the Ember Way is, for any given combination of APIs. For instance, what are best practices before you enable outer-html, but after you switch to using .args? What are best practices before you enable outer-html, but after lifecycle hooks have been deprecated? What are best practices when using classic class syntax with all these new features?

Doing this incrementally, feature by feature, necessarily increases the possible permutations of in-between states, and makes it harder to define conventions for each one of them. New users may get lost in the variety of features (and permutations of them) they need to learn, and existing users may be frustrated by the lack of clarity.

Interoperability

One of the stated goals of Glimmer components in the RFC is to interoperate with Glimmer.js, and allow users to begin using components with both frameworks. On its own, this is definitely not a compelling enough reason to make the decision to have a new default component class - Ember should do what's best for Ember. However, with the other reasons pushing us toward a new base class anyways, it is a nice win that will enable more experimentation, and increase Ember's appeal to users who care about byte-size. Without a new base class, we definitely would not be able to do this for some time, since it would require removing all Ember specific parts of the component class (including extending from EmberObject).


```ts
interface GlimmerComponent<T = object> {
args: T;
Copy link
Contributor

Choose a reason for hiding this comment

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

I've always wanted to have something like React's PropTypes in Ember. More than that, I really wanted to be able to specify the attributes a component should receive with Typescript. By specifying the shape of the args using Typescript, could we get typed arguments in Glimmer components?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, the answer is partially. You definitely will be able to use TS to specify the shape of args when defining classes, and internally within the class these types will work. However, we don't currently have a way to integrate templates with the Typescript compiler, so there's no way for us to validate that the types are correct when a component is used in a template. Long term, I think this is the ideal way to have argument typings.

In the meantime, runtime validations like React's PropTypes are possible, but extracting them from TS would be tricky. There's been some discussion of this on Discord, but nothing concrete as of yet. Libaries like @ember-decorators/argument and ember-prop-types can continue to fill the gap, but I definitely think we can do more here!

Copy link
Contributor

Choose a reason for hiding this comment

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

So, if understanding you correctly, this RFCs is does some the work to support typed args but it's not all the work that needs to be done to validate that a component is being passed the right arguments. In other words, this RFCs lets us specify the types a component should receive but we can't actually validate we're sending those types because the templates aren't integrated into the typescript compiler. Is that right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but this is a nice incremental step forward. It also allows the usage of this.args inside the component to be properly typed which is a nice win too...

@michaelrkn
Copy link

We should definitely add a section addressing this in Alternatives, but to answer directly

Yeah - you could even just copy your comment over!

There is no middle state for most of these features.

Can't ES6 classes, .args, and outer HTML be implemented independently from each other?

This flag would essentially disable a whole bunch of hooks and properties (all of the event hooks...

Why would all of the event hooks need to be disabled in order to use outer HTML? With ember feature:enable template-only-glimmer-components, can't users get outer HTML with the current event hooks?

Glimmer components having a much smaller API will be immediately better for performance, as will them not inheriting from EmberObject.

What is the performance impact? And isn't a major version release a better place to focus on removing APIs?

@pzuraq
Copy link
Contributor Author

pzuraq commented Dec 23, 2018

Can't ES6 classes, .args, and outer HTML be implemented independently from each other?

Yes, the absolutely can. What I was referring to here was the set of features that are tied together by Outer HTML, so:

  • tagName
  • classNames
  • classNameBindings
  • attributeBindings
  • Event hooks such as click, focus, hover, etc.
  • Default bound properties such as isVisible, ariaRole, etc.
  • The element property, and any usages of it

There may be some more I'm forgetting at the moment, but the point is that as soon as you toggle that switch, you have to rewrite all of the above to the new world. That's a fairly big switch, and really points to two separate class APIs.

Why would all of the event hooks need to be disabled in order to use outer HTML?

I was specifically referring to the click, focus, hover etc. style event hooks here, not the lifecycle hooks like didInsertElement and didRender. Event hooks would not be able to keep working since there is no element to apply them to. Lifecycle hooks would be able to continue functioning, but would likely need to be rewritten since you can't use element anymore.

What is the performance impact?

It's hard to say without actual benchmarking, and it's hard to benchmark without rewriting a real world application 😄 We do have a minimal component API in the form of sparkles-component now so we can start to do this to get a real world estimation.

However, the fact is that we are doing much more work than is necessary right now, between the usage of EmberObject and the many, many lifecycle and event hooks of classic components. Components are one of the most commonly instantiated objects in an Ember app, so even the smallest wins here should move the needle.

And isn't a major version release a better place to focus on removing APIs?

Absolutely! This is not a deprecation RFC, and classic components will remain part of the Ember API for the forseeable future. They still have functionality which is not available in Glimmer components (such as positional parameters), and they are very widespread in usage so it will take a long time to transition away from them.

This was part of the reason for creating component managers in the first place, and the painstaking design that was done to ensure that all forms of component defined using them would interoperate at all levels - to ensure that transitions could be done incrementally, without removing or deprecating component APIs that are still in common use.

@michaelrkn
Copy link

@pzuraq Thanks for all the explanation. I'd still suggest that .args be added to classic components, so that current Ember users have a clearer path forward: switch to .args, switch to ES6 classes, and finally switch to outer HTML and Glimmer.

@NullVoxPopuli
Copy link
Contributor

@michaelrkn that'd be a good thing to do in a deprecation RFC - for helping out with the transition and such


Glimmer components align their argument access with named args by making
arguments available exclusively on a (shallow) frozen object, `this.args`.
Attempting to modify `this.args` will hard error, meaning that like templates,

Choose a reason for hiding this comment

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

Apologies for this question may be too small. What's

Attempting to modify this.args will hard error, ....

I guess you are trying to say something like 'will throw error' right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, exactly!

@scottmessinger
Copy link
Contributor

Does this RFC for Glimmer Component prevent introducing single file components in the future? React pioneered this and since then, it's been picked up by Svelte and Vue.js (among others). Personally, I only use single file components with Ember and it's glorious:

import hbs from "htmlbars-inline-precompile"

export default Ember.Component.extend({
    // ....

   layout: hbs`
     <h1>Template here</h1>
   `
})

For me, it makes the app way less confusing to navigate as I'm no longer juggling 2 files for every one component. My feelings aside, the adoption of single file components in both React, Vue.js, and Svetle.js suggests other developers really love the simplicity this provides.

With the removal of layout, the way I do things now won't work and a new way of integrating the template and component definition would have to be developed. I know this RFC isn't addressing single file components, but does anything in the RFC seem like it would prevent templates and components from being in the same file in the future?

@cibernox
Copy link
Contributor

@scottmessinger I believe it doesn't prevent single-file components. It could just be implemented with babel transform that takes out the template: hbs... bit and treats it like a different file.

As I read it what this DOES prevent is something like reusing a single template file for several components, is that so? Maybe the same thing could be achieved creating a symlink?

@rwjblue
Copy link
Member

rwjblue commented Dec 24, 2018

Does this RFC for Glimmer Component prevent introducing single file components in the future?

No, this RFC definitely does not foreclose on the concept of single file components (which is definitely something that we would like to push forward in the near term).

import hbs from "htmlbars-inline-precompile"

export default Ember.Component.extend({
    // ....

   layout: hbs`
     <h1>Template here</h1>
   `
})

It does however make the specific syntax that you highlighted not work (because there would no longer be a layout property). The single file component world would use a more customized syntax so that both the JS/TS and template can be properly highlighted...

@rwjblue
Copy link
Member

rwjblue commented Dec 24, 2018

As I read it what this DOES prevent is something like reusing a single template file for several components, is that so?

No, this shouldn't be an issue as far as I can tell. In the event that you wanted to use the same template in multiple places, you would author it in the "primary" location and use a reexport in the various other locations...

@rwjblue
Copy link
Member

rwjblue commented Jan 11, 2019

We reviewed this RFC at the core team meeting today, and believe that this is ready for final comment period.

@courajs
Copy link

courajs commented Jan 12, 2019

I love all the detail here, especially in the teaching section.

What's the rationale on needing both isDestroying and isDestroyed? Is there a good motivating example to share?

@pzuraq
Copy link
Contributor Author

pzuraq commented Jan 13, 2019

@courajs this is actually based on Ember's runloop and meta system. When you destroy an Ember object, you are also tearing down a companion meta object. If this isn't done today, it's possible that your app can have memory leaks due to references that the meta objects keep (though this will probably become less and less of an issue over time as we move away from certain abstractions).

All object destructions in a single runloop are scheduled to occur together, at the end of the runloop, since the teardown work is very similar and more efficient if done together. So, today, when you call obj.destroy() (where obj is an EmberObject), we set isDestroying to true, then schedule the actual destruction to occur later. This way, your code can be aware that the object will be destroyed in the current runloop, but hasn't been fully destroyed just yet.

Glimmer Components won't extend from EmberObject, but they will use the same meta system for computed properties, tracked properties, and other things. Keeping the destruction flow the same will make it easier for code that needs to interoperate with both components, and there isn't really a good motivating reason to change it, so we opted to keep the same flow. Hope that answers your question!

edit: Also, FWIW, something we've discussed is exposing these functions globally so that you can use them with POJOs and plain classes that also use these meta objects. So for instance, something like:

import { destroyMeta, isDestroying, isDestroyed } from '@ember/meta';

obj = {};

destroyMeta(obj);
isDestroying(obj); // true
isDestroyed(obj); // false

setTimeout(() => {
  isDestroyed(obj); //true
});

I think this is worth its own RFC if we were to do it. IMO, ideally, you shouldn't have to worry about destroying metas, and we would just allow them to be garbage collected naturally when an object is no longer in use.

@mike-north
Copy link
Contributor

Could we add a bit to "how we teach this" that addresses @glimmer/component templates having no backing object, where @ember/component and sparkles-component templates having one?

One snag I've already seen experienced devs hit as they adjust to the "octane mental model" is the use of {{action (mut x) }} with implicit state.

The following (template-only) Ember.Component component will work as you'd expect, allowing the user to toggle whether content is hidden/shown with respective buttons.

{{#if showSomething}}
  [Here's your something]

  <button onclick={{action (mut showSomething) false}}>Hide</button>
{{else}}
  <button onclick={{action (mut showSomething) true}}>Show</button>
{{/if}}

This will not work the same way with glimmer components, since with no backing object, showSomething would no longer be an implicit value that's initialized to undefined.

related: emberjs/ember.js#17202

@pzuraq
Copy link
Contributor Author

pzuraq commented Jan 13, 2019

@mike-north Glimmer Component's do have a backing object, and patterns with mut and such will continue to work the same as they always have. Template-only components, which were proposed in #278, do not. I think there may be some confusion here since template-only-components are pure Glimmer VM components, without a state bucket or backing class, and I don't believe the guides have been updated to cover them since they are an optional feature that will remain optional until Ember@v4.

We should definitely add a section thoroughly explaining template-only components if the optional feature is turned on by default in Octane (I think it will be, but would have to confirm), but I'm not sure if it belongs here since this RFC is about a new backing class for components, and any components which use that class will necessarily not be template-only.

@mike-north
Copy link
Contributor

@pzuraq I'd argue that "Glimmer Components" vs "Glimmer VM Components" is an implementation detail.

In the GC world, deleting my .js or .ts file would result in one of two things, depending on whether the flag that's currently enabled in the blueprint, remains so.

A (template-only-glimmer-components = false) - the template is treated as a template-only Ember.Component. Lifecycles will work differently, {{action 'foo'}} is fair game, inner-html mode, etc...

B (template-only-glimmer-components = true) - the template is treated as a template-only Glimmer VM component. No more state bucket or backing object.

Both of these involve the component's nature changing more than developers may be used to (compare to the classic mental model of "you can safely delete empty JS modules" for components, routes, controllers, adapters, serializers). This is something that needs special teaching attention to set devs up for success.

@pzuraq
Copy link
Contributor Author

pzuraq commented Jan 13, 2019

True, and I understand your concern - this change in behavior needs to be documented. However, this was discussed and decided upon in a previous RFC. If it were not for #278, we could for instance have decided to continue creating a backing component for template-only-components, and just have changed it to be an instance of GlimmerComponent. I think the most appropriate place to discuss the impact on guides and learning for removing the backing class from template-only-components would have been #278.

That said, Octane (and more generally, a new Edition) is about rolling up the various changes since the last edition and making sure they are all fully covered in the guides. This definitely includes TO-components, along with Angle Bracket invocation, named args, and Glimmer Components. I think ideally we would have done a better job discussing the learning impact when TO-components were first proposed, but they will most certainly be covered by the effort to update the docs in this new edition.

@richard-viney
Copy link
Contributor

richard-viney commented Jan 16, 2019

This RFC is looking great! Handling of component argument defaults is the one thing I'm still a little uncertain about.

My thoughts are:

  1. Using the {{or}} helper buries the default in an unknown place in the template, and requires it to be duplicated if the arg is used multiple times.
  2. A defaulting getter or an @arg decorator are better, though my take is that neither are technically an argument default, because they're no longer referenced using the @ syntax in the template.

Has any consideration been given to some kind of defaultArgs() method? E.g.

interface Args {
  enabled: boolean
};

export default class extends GlimmerComponent<Args> {
  static defaultArgs(): Partial<Args> {
    return { enabled: true };
  }
};

Any arguments that aren't provided to the component, or have the value undefined, would have the default applied. The @ syntax in the template can be used to access an argument that had a default value applied. This could be limited to primitive number/string/boolean/null values to avoid component instances ending up with references to the same JS object in their args.

I realise the above may cut against the intended role of @ arguments somewhat.

@pzuraq
Copy link
Contributor Author

pzuraq commented Jan 17, 2019

@richard-viney that's a great point, and while I do believe it has been discussed somewhere I can't seem to find it in this PR so I think it'll be good add the reasoning behind why we didn't add defaultArgs here.

From a language design perspective, it comes down to being able to look at the template and know, with confidence, what things are. Currently, with all of the new features, we can know that:

  • {{@arg}} refers to the value passed into the component, <Foo @arg="value">
  • {{this.field}} refers to a field/property on the component
  • {{item}} refers to either a global value (helper, modifier, etc) or a local variable ({{#each items as |item}})

Note: item still can technically also refer to the property on the component instance, but not using this has been deprecated.

We can know these things about these values without even looking at the rest of the template, which is pretty cool when you consider how dynamic Handlebars templates used to be! It used to be that you had no idea whether or not something was a property, an argument, a local variable, a global, etc. without reading not only the entire template, but also the component.js, since arguments were properties.

With {{@arg}} syntax as it is currently, this is even better. We can actually skip around and read templates without having to read any javascript, because we know for certain that whenever we see {{@arg}}, it's referring directly to the value passed in on template invocation. There is no way the component.js could have affected the value, so we don't even need to check. Personally, I like to be able to read templates when getting to know a new application and get a general understanding of how things work, so this constraint helps a lot.

The fallback is that we use an alias with a default, so something like {{this.argWithDefault}} let's say. DX-wise, this is no better or worse than defaultArgs. We still have to go to component.js to see what the value is, and we now know that from a hint in the template. So, keeping the constraint doesn't really harm DX, it just feels a little strange.

One final note is that one frequent use case for default arg values is for subclassing different types of components and overriding default values:

<!-- button.hbs --> 
<button class="{{this.buttonClass}}">{{yield}}</button>
// button.js
export default Component.extend({
  buttonClass: 'default',
});
// success-button.js
export default Button.extend({
  buttonClass: 'success',
});

We believe this is actually better done (and probably more performant) "functionally", via templates:

<!-- button.hbs --> 
<button class="{{or @buttonClass 'default'}}">{{yield}}</button>
<!-- success-button.hbs --> 
<Button @buttonClass="success">{{yield}}</Button>

This can quickly become a maintenance nightmare however, as soon as you add a new arg to Button it needs to be passed through each type of button. We're currently discussing adding a ...arguments syntax, which would mirror the ...attributes syntax that currently exists, and would cover that edge case (and in general make forwarding arguments much easier).

That covers the language design part of this, but the other part is that the {{@arg}} syntax currently completely bypasses the component instance, and goes directly to the arguments in the template layer. Via the Custom Components RFC, the component manager also cannot mutate the arguments object in the template layer, so there really isn't a way to do it, and it would be a pretty significant engineering effort to make it possible (if it could even be done a backwards-compatible way).

@rwjblue
Copy link
Member

rwjblue commented Jan 17, 2019

@richard-viney - Thanks for chiming in! Unfortunately, I think you hit the nail on the head here:

I realise the above may cut against the intended role of @ arguments somewhat.

😺

Specifically, a named argument in the template tells you explicitly that that value is unaffected by the backing JS/TS file for the template. If we made it possible to modify/mutate the value of the named argument in the JS/TS file that pretty squarely violates the original goal. The motivation section of the Named Arguments RFC has more explanation and details here.

Using the {{or}} helper buries the default in an unknown place in the template, and requires it to be duplicated if the arg is used multiple times.

In lots of cases where you have a simple default, this is a perfectly good solution when you don't also need the defaulted value in the JS/TS side. RE: the caveats:

  • Not all templates will need to default multiple times (and you could use a {{#let to share a single value if you do).
  • I don't think putting template defaults in the template is burying it 😜. Contrarily, I think putting a defaulting mechanism in the JS/TS file when its only needed in the template is worse (barring some exceptional cases where the defaulting is needed in the JS/TS for other reasons, the logic is complicated and having it in JS/TS makes it easier to reason about and test, etc).

A defaulting getter or an @arg decorator are better, though my take is that this neither are technically an argument default, because they're no longer referenced using the @ syntax in the template.

Yes! I think this is absolutely the right thing! Seeing {{this.foo}} in the template tells the reader of the template to "go look in the JS/TS file backing this template". That is precisely the message you should be sending when you need to use the JS/TS file to gather a default value.


In a future world (when Property Lookup Fallback Deprecation and "template imports" are done) there will be three different ways of referencing values in a template:

  • {{foo}} -- this will only mean a local value defined in the template itself (ala block param)
  • {{this.foo}} -- this will only mean "lookup on the backing context" (aka look in the component's JS/TS file)
  • {{@foo}} -- this will only mean "look at the callsite to track down what is passed in"

IMHO, this massively clarifies templates for future readers, and reduces a number of super common pitfalls in the current system...

@rwjblue
Copy link
Member

rwjblue commented Jan 17, 2019

@pzuraq haha, our replies crossed in the mail! I think you probably said it better though 😃

@richard-viney
Copy link
Contributor

richard-viney commented Jan 18, 2019

Thanks for the responses!

Yes I've read the Named Args RFC carefully, have been using them for some time in production, and generally agree with the rationales put forward there.

  • Not all templates will need to default multiple times (and you could use a {{#let to share a single value if you do).

I agree that for arguments which only need a default applied once in the template it's reasonable to apply the default in the template directly. Is using {{or}} actually going to be recommended practice though? Won't this cause surprises when the argument value passed in is falsey/blank? A new (very simple) helper may be needed instead of {{or}} for this template-based defaulting behaviour.

Wrapping a template in {{#let}} in order to apply argument defaults feels a tad awkward to me - is this being suggested as a best practice? Such defaults obviously aren't available in the component JS/TS, and a developer can still easily use the @ version of the argument without realising they actually needed the defaulted version as available in the {{#let}} block.

  • I don't think putting template defaults in the template is burying it 😜.

By 'burying' what I meant was: how does a developer coming to a component know where to find the default value for an argument if it has one? i.e. they're interested in the component's interface, rather than diving into its internals. The most immediately useful thing to them is the Args interface (TS-specific, sorry) as it defines the component's primary public interface. Default values of non-optional args are relevant information here too.

As an aside I've found that documentation of a component's public interface (including defaults) is complicated a little by template-only components. The team needs a different convention for documenting its available args (and their defaults). For backed components this is handled fairly naturally in the backing TS/JS file. Are there other documentation conventions for components that I'm not aware of?

Contrarily, I think putting a defaulting mechanism in the JS/TS file when its only needed in the template is worse (barring some exceptional cases where the defaulting is needed in the JS/TS for other reasons, the logic is complicated and having it in JS/TS makes it easier to reason about and test, etc).

My experience is that it's common for a component to have some args that are only consumed in the template, some args that are only consumed in the JS/TS, and some args that are consumed in both places. Splitting responsibility for defaults such that args only referenced in the template are defaulted there, and args that are referenced in the JS/TS are defaulted there in some other manner seems sub-optimal, ideally it would all be handled in a unified way. Also args may switch where they're being consumed (e.g. when refactoring) which complicates matters if defaults aren't handled via a single system.

Some other possible discussion points:

  • Using this.args inside the JS/TS to access an argument value becomes possibly the wrong thing to do, as there may be some other property that should be used to get a necessary default value applied.
  • Similarly the @ syntax becomes possibly the wrong thing to do in the template, and it's quite hard for a dev to know that.
  • The code example I gave with an arg called enabled that defaults to true seems like a plausible situation developers may implement - so what do we recommend to them? It seems a shame to have to backtrack on great conventions such as use @/this.args when accessing argument values just because a simple default value of true is needed.

I can't speak directly to the implementation/architectural complexity here - though I recognise it is an important consideration. Would it still be a significant challenge if the 'defaults' object was immutable and was provided once on registration of a component type? The main point here is that there would be no need to access the component instance when falling back to a default argument value. I wasn't intending to suggest that individual component instances be able to customise the defaults in any way, they would apply globally to all instances.

Sorry for the exposition! I've been following the project for a while and felt compelled enough by this to chime in.

@pzuraq
Copy link
Contributor Author

pzuraq commented Jan 18, 2019

I think you make some great points here. A few thoughts here:

Is using {{or}} actually going to be recommended practice though? Won't this cause surprises when the argument value passed in is falsey/blank?

FWIW, I think that this behavior is actually desired in many cases, specifically when a value is null or undefined. I do think that this should be handled by a {{nullish}} helper, but my point there is that defaulting is not always as simple as "use this value if I wasn't passed anything", and while I think providing some default logic may be convenient, I also believe we will likely see workarounds for many of these cases (as we do today).

The most immediately useful thing to them is the Args interface (TS-specific, sorry) as it defines the component's primary public interface. Default values of non-optional args are relevant information here too.

Exposing the expected interface of the component in a unified way definitely is important, agree 100% here. I do think that this is a somewhat separate problem from the question of defaults though, specifically because this problem is mostly related to TO-components (since components with a class do have a way to define this interface, as you pointed out), and one of the major benefits of TO-components is that they do not instantiate a class, making them more performant than components with a backing class. I think whatever mechanism we use to define this interface, it shouldn't encourage/require creation of a class just to define it.

Splitting responsibility for defaults such that args only referenced in the template are defaulted there, and args that are referenced in the JS/TS are defaulted there in some other manner seems sub-optimal

I'm not sure I agree here. I think splitting the interface definition is definitely sub-optimal, but the defaulting logic could actually be better being split because it means that the default values are shown where they are relevant, and don't require context switching to find.

A few final thoughts here:

  • I expect most defaulting to occur in a standard way in the component definition, except in very simple cases. {{this.argOrDefault}} will be pretty standard in most apps, and there are already standardized solutions popping up for this. From a language perspective, this is not any worse than {{@arg}} if {{@arg}} could be defaulted. We can see that {{this}} is referenced, so we must go to the component definition, and upon doing so we see that it is an argument alias with a default.

    Conversely, if {{@arg}} can be defaulted, we do lose the guarantees we talked about before. We must now always check the component definition to know if there was a default applied. So, allowing {{@arg}} syntax to be defaulted is strictly worse for {{@arg}} syntax, while the alternative of using aliases is pretty comparable.

  • A defaulting interface could be added at a later point if we find that it is very difficult for developers to work without one. This would give us more time to figure out design details, such as how it would work for TO-components (maybe it's a function or exported const instead of a method on a class?) and would allow us to experiment with different approaches, and figure out how to best integrate it into Glimmer-VM. I think this RFC can move forward as it stands, and we can continue to discuss defaults and develop solutions in the community. If we rush it, we'll be stuck with it.

  • I think there could be more novel approaches for defaulting in templates that could help here, for instance maybe a default-args helper:

    {{default-args
      @arg1="default text"
      @arg2=true
    }}
    
    <div selected={{@arg2}}>{{@arg1}}</div>

    This could be accomplished today with template transforms, similar to ember-let, allowing us to experiment without committing to a particular design.

@richard-viney
Copy link
Contributor

One point to note is that having a way to specify defaults in the JS/TS file doesn't have to mandate a backing component class/context, along with its potential performance drawbacks. Building on what you suggested, we could envision a backing file such as

export interface Args {
  enabled: boolean
}

export const defaultArgs: Partial<Args> = {
  enabled: true
}

Here, the JS/TS file doesn't export a component class at all. This would allow defaults to be specified in a unified way across both normal and TO components, and has other benefits such as a unified place to define/document the component interface that works for TO components too, which I think is highly valuable. It also allows for proper type checking in TO components in future when type-safety in templates is a thing, which wouldn't be possible in current TO components as there'd be no Args interface to type against.

Another observation is that the issues raised re. avoiding context switching between the template file and the backing file when reading the code are already going to reduced: first by MU, and then entirely by single-file components (when available). If we take both of these as broadly desirable changes then the concern re. context switching (which is mentioned frequently) would go away. At this point it would be hard to champion any defaulting constructs that were either specific to the use of arguments in templates (which could cause confusion due to divergence from the values held in this.args), or that required a backing class for the component (which has performance implications). Hence why the export const defaultArgs solution may have some merit.

An additional nicety here is that adding a backing class to a previously TO component becomes less potentially error-prone, as there'd be no figuring out of what arg defaults may be being applied in the template that now need to be moved to the backing class to be used in the JS/TS. Instead you just export the new backing class and continue with this.args. and @.

I considered suggesting a {{default-args}} style construct in the template like you mention, but it being template-specific made it less appealing (similar to the {{#let}} idea) as the end goal I'm shooting for here is a unified defaulting approach across both the template and the backing class (if one exists).

FWIW, I think that this behavior is actually desired in many cases, specifically when a value is null or undefined. I do think that this should be handled by a {{nullish}} helper, but my point there is that defaulting is not always as simple as "use this value if I wasn't passed anything", and while I think providing some default logic may be convenient, I also believe we will likely see workarounds for many of these cases (as we do today).

If we follow JS convention for functions with default arguments then an argument with a value of undefined would use its default value if one was provided. An alternative would be to apply the default only when the component invocation does not specify a value for the argument, which I assume Glimmer can detect. Personally I'd vote for the former for parity with JS function argument defaults.

A defaulting interface could be added at a later point if we find that it is very difficult for developers to work without one. This would give us more time to figure out design details, such as how it would work for TO-components (maybe it's a function or exported const instead of a method on a class?) and would allow us to experiment with different approaches, and figure out how to best integrate it into Glimmer-VM. I think this RFC can move forward as it stands, and we can continue to discuss defaults and develop solutions in the community. If we rush it, we'll be stuck with it.

Agreed, most (all?) of what's being suggested around handling defaults is an addition rather than a fundamental change to the overall Glimmer components RFC, and could be handled in a follow-up RFC based on broader experience and feedback on pain points. We can certainly make do in the meantime with @arg and co.

@rwjblue
Copy link
Member

rwjblue commented Jan 22, 2019

Thank you @richard-viney and @pzuraq for hammering through the ideas around defaulting and concluding that it is severable into a follow up RFC!

@rwjblue
Copy link
Member

rwjblue commented Jan 22, 2019

This RFC was moved into Final Comment Period a little over a week and a half ago, all of the concerns that were raised have been addressed (or will be layered on top of this one in a new RFC): its time has come. 😸

Thank you to everyone who participated in the process, we absolutely would not have been able to do it without y'all!

@rwjblue rwjblue merged commit a0d5f30 into emberjs:master Jan 22, 2019
@mike-north
Copy link
Contributor

Will @glimmer/component continue to provide type information as part of its public API?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Final Comment Period Octane T-framework RFCs that impact the ember.js library
Projects
None yet
Development

Successfully merging this pull request may close these issues.