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

[WIP] Glimmer Components #1

Merged
merged 4 commits into from
Feb 7, 2019
Merged

[WIP] Glimmer Components #1

merged 4 commits into from
Feb 7, 2019

Conversation

pzuraq
Copy link
Owner

@pzuraq pzuraq commented Nov 30, 2018

No description provided.

text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
Copy link

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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


I'd like to see a typescript interface for the final class design. The prose is great to rationalize / explain, but it makes it hard to get a complete shape of the interface. I think it really is quite tiny:

interface Component {
  args: unknown;
  constructor(owner: Owner, args: unknown): void;
  didCreate(): void;
  didUpdate(): void;
  destroy(): void;
}

Great job, this looks really good! 👏

text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Show resolved Hide resolved
This makes the `element` property of legacy components awkward, since there is
no one single reference-able element. A previous proposal for solving this
problem was a `bounds` property which contained `firstNode` and `lastNode`, but
this could be very confusing to users since these nodes would likely have to be
Copy link

Choose a reason for hiding this comment

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

There was a much more fundamental flaw in the bounds concept than just user confusion. If all you received was bounds, and you wanted to setup something on an element, you have no way to ensure that thing was torn down when the element was removed.

Take for example:

{{#if something}}
  <p>Hello world!</p>
{{/if}}

Imagine you were grabbing that p via bounds to setup something, well you have no way to know when you need to tear that down.


Tldr; it was very error prone...

Copy link
Owner Author

Choose a reason for hiding this comment

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

To be fair I think this is also true of didInsertElement style components too, you would have to create a component within the if to get a solid place to contain lifecycle hooks.

text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved

```html
<!-- OUTPUT -->
<div>
Copy link

Choose a reason for hiding this comment

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

Worth including the id="ember-1234" and class="ember-view" gunk here? That adds to the problem, and the overall noisiness, quite a bit.

tagName: 'section',
classNames: ['post'],
classNameBindings: ['type'],
ariaRole: 'region',

Choose a reason for hiding this comment

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

tiny note- section already has the role of region by default, so it doesn't need to be explicitly defined. Reference: https://www.w3.org/WAI/PF/aria/roles#region

Copy link
Owner Author

Choose a reason for hiding this comment

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

I actually meant to ask you if the aria stuff made sense, but forgot. What would a good role for this be you think? Something to demo the old attribute bindings vs the new outer HTML semantics

* called _after_ any child element's `{{did-render}}` modifiers
* called in definition order in the template
* **May or May Not**
* be called in the same tick as DOM insertion
Copy link

Choose a reason for hiding this comment

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

Seems tricky to say this isn't guaranteed in practice, since changing it later will almost certainly break app code, right?

Copy link

Choose a reason for hiding this comment

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

Relatedly, there are different use cases for accessing a DOM element before layout and after layout. If you want to do some kind of setup or initialization (e.g. a jQuery UI control), you want to do it before layout/paint to ensure there's no ugly flash of empty content. But if you want to do some sort of measurement (some behavior based on the element's natural width or height, for example) you want to do this after layout.

Today people typically have to learn about afterRender if they want the post-insertion, post-layout element. Would this be better modeled as a different element modifier/lifecycle hook?

Copy link

@rwjblue rwjblue Dec 11, 2018

Choose a reason for hiding this comment

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

@tomdale if what you are doing is based on the DOM, you should be using the element modifiers which do give you the guarantees that you are mentioning...

Copy link
Owner Author

Choose a reason for hiding this comment

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

@tomdale the guarantees we list here were specified in the element modifier manager RFC. I think we'd have to change them there, this is really just reiterating the timing to make it clear how everything works with everything else.

Re: timing relative to layout/paint, I'm unsure. I think this would make sense as a separate modifier, but in my experience this type of work is generally best done in a requestAnimationFrame (it's incredibly cheap to measure there, very expensive to mutate), but I am not at all an expert on it.

These modifiers aren't meant to solve every edge case, for sure. They are trying to solve the 95% cases, mostly the roles didInsertElement, didRender, and willDestroyElement fulfilled.

Choose a reason for hiding this comment

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

I don't think there is anything to debate here... we merged the RFC that specified the timing of this.

The modifiers are installed pre-layout. This is why I'm unsure about calling this {{did-render}}. The modifier installation is scheduled and is called from bottom-up in the commit of the render transaction. Because of this I'm not sure if {{did-insert}} is any better because you "inserting the modifier" not the element.

If you wanted a modifier to measure something you would need to do and afterRender in the installModifier hook.


```hbs
<div {{will-destroy (action this.teardownElement)}}>
```
Copy link

Choose a reason for hiding this comment

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

I think it would be worth having some examples, either here or in the "How we teach this" section, that cover how you'd model common uses of didInsertElement with the new element modifiers. I think there are some particularly cool examples that highlight the benefit of this approach, like conditionals (as already mentioned here) as well as wanting to apply some DOM mutation to multiple items in a list.

Choose a reason for hiding this comment

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

In particular, I'm very interested in plans for bridging to a mental model where elements have their own lifecycles, and the interplay with the component's lifecycle around them

i.e., if you have a set of four tabs, each with a D3 visualization, do you really want things torn down and initialized as you go from tab-to-tab? Maybe we need to have some increased emphasis on making good decisions around use of {{#if, visibility: none, etc... and the ramifications on element lifecycles

<!-- templates/components/post.hbs -->
<section ...attributes role="region" type={{@post.type}} class="post {{@post.type}}">
{{#if (eq @post.type 'image')}}
<img {{did-render this.didInsertImage}} src={{@post.imageUrl}} title={{@post.imageTitle}} />

Choose a reason for hiding this comment

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

I'm wondering if {{did-render}} should be called something different. If I remember correctly the timing of this would be "installed on element but not painted". I also don't know if the state of the element is "in-dom". Might be worth checking.

Copy link

Choose a reason for hiding this comment

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

definitely open to naming suggestions here, it's also different enough from the didRender lifecycle hook on classic components that we may want to distinguish it

text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
In classic components, arguments are set as properties directly on the class
instance. This means that class methods and properties can be completely
overwritten by incoming arguments, which can have surprising and problematic
side effects. For example, let's say we have a component that has a

Choose a reason for hiding this comment

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

This means that class methods and properties can be completely
overwritten by incoming arguments, which can have surprising and problematic
side effects.

This isn't always problematic. The same phenomenon can be helpful for default values

  {{cart-item price=5.25 }}
  {{cart-item price=1.99 qty=3 }}
export default Ember.Component.extend({
   qty: 1,
   total: computed('price', 'qty', function() {
     return this.price * this.qty;
   })
});

Copy link
Owner Author

Choose a reason for hiding this comment

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

This is true, and something we should explicitly address in drawbacks.


Someone may decide they want this component to add middle names to the full
name too. However, instead of updating the component, they decide to override
the method instead:

Choose a reason for hiding this comment

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

In teaching thousands of developers Ember, I've literally never seen someone try to do this, with the exception of something like formatter functions (x => '$' + x) that are meant to be passed as arguments.

I feel like it's contrived, to the point of being inappropriate when framed as one of the primary motivating factors for this RFC.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I updated this to be less contrived, but as we noted in conversation, this example is from my own (particularly painful) experience 😄

component's implementation are not truly private. In most common day-to-day
scenarios, it means that developers have to tread carefully to make sure they
don't accidentally overwrite a critical function or property, and occasionally
it can be misused.

Choose a reason for hiding this comment

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

Again, framing this as a problem that plagues developers "day-to-day" causing them to feel like they must "tread carefully" may be inappropriate.

I'm totally off base here, please provide me with some examples in real projects so I may correct my misconception.

it can be misused.

Glimmer components assign their arguments to the `args` property on their
instance, preventing namespace collisions from happening in the first place.

Choose a reason for hiding this comment

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

Might be worth calling out what happens for

<Person @args={{this.val}}> <!-- this.args.args -->

or

<Person args={{this.val}}> <!--   <div args="foo"></div>  -->

to help the point land RE: namespacing and regular DOM attributes

text/0000-glimmer-components.md Show resolved Hide resolved
text/0000-glimmer-components.md Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Show resolved Hide resolved
that a user provided value or a default/mutated/computed value?" becomes "Was it
an argument or not?"), and encourages use of `{{@arg}}` syntax in templates
where appropriate. At scale, this makes reading templates even easier, since
more information is encoded in the template itself.

Choose a reason for hiding this comment

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

Needs a bit of a rewrite, to account for non-named-arguments.

For me, it's easy to accept immutable arguments as a clear win except for one case: "value editor" components (i.e., a color picker).

For "value editor" components, I would still find myself wanting to "push" changes upstream to the state owner via two-way binding, since it's effectively a custom {{input value=foo }}.

Some inspiration for the solution to this pattern: https://reactjs.org/docs/forms.html#controlled-components


Glimmer components will only receive the owner directly, and as such will _not_
support type injections. This cuts down on the implicit knowledge developers
must have when writing a component.

Choose a reason for hiding this comment

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

Sounds like if I need to use getOwner a component member function, it's my responsibility to hold onto a reference.

class XFoo extends Component {
  constructor(protected owner: ApplicationInstance) {}
}

text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Show resolved Hide resolved
text/0000-glimmer-components.md Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
text/0000-glimmer-components.md Outdated Show resolved Hide resolved
but they run on elements instead of components.
* `{{did-render}}` is similar to the `didRender` lifecycle hooks in classic
components. It will run when the element is inserted, and then it will run
again when any of its _args_ change.

Choose a reason for hiding this comment

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

I have some concerns about the relative complexity of patterns like the twitter bootstrap 3 tabs. It may grow significantly relative to the classic didInsertElement way of doing things.

Developers create a HTML structure like

<!-- Nav tabs -->
  <ul class="nav nav-tabs" role="tablist">
    <li role="presentation" class="active"><a href="#home" aria-controls="home" role="tab" data-toggle="tab">Home</a></li>
    <li role="presentation"><a href="#profile" aria-controls="profile" role="tab" data-toggle="tab">Profile</a></li>
    <li role="presentation"><a href="#messages" aria-controls="messages" role="tab" data-toggle="tab">Messages</a></li>
    <li role="presentation"><a href="#settings" aria-controls="settings" role="tab" data-toggle="tab">Settings</a></li>
  </ul>

  <!-- Tab panes -->
  <div class="tab-content">
    <div role="tabpanel" class="tab-pane active" id="home">...</div>
    <div role="tabpanel" class="tab-pane" id="profile">...</div>
    <div role="tabpanel" class="tab-pane" id="messages">...</div>
    <div role="tabpanel" class="tab-pane" id="settings">...</div>
  </div>

and then run (w/ optional jQuery enabled)

$('#myTabs a').click(function (e) {
  e.preventDefault()
  $(this).tab('show')
})

We have two sibling elements that sort of relate to eachother, and are "activated" by some foreign JS plugin. It doesn't feel entirely correct to put a modifier-based hook on either <ul class="nav nav-tabs" role="tablist"> or <div class="tab-content"> since it's the pair of them that's important, and neither is the parent of the other.

Is our answer here to add a wrapping element for the sole purpose of having a common parent of the two siblings which must be set-up/destroyed together?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think in this case it would make sense to place the modifier on just the tabs, since they are controlling the content, and the modifier would be setting up the control. A wrapping div would not be necessary, though if users felt it made more sense they could use it.

but they run on elements instead of components.
* `{{did-render}}` is similar to the `didRender` lifecycle hooks in classic
components. It will run when the element is inserted, and then it will run
again when any of its _args_ change.

Choose a reason for hiding this comment

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

In moving rendering lifecycle hooks out of the JS/TS module and into the template (attached to elements), we make development of element-less components more challenging.

Example: ember-google-maps allows a declarative google maps configuration to be built in handlebars w/ syntax like

{{#g-map lat='51.508530' lng='-0.076132' zoom=10 as |g|}}
  {{#each locations as |l|}}
    {{g.marker lat=l.lat lng=l.lng onClick=(action 'showDetails' l)}}
  {{/each}}
{{/g-map}}

Only the outer-most component renders anything (a single element -- a <div> I think) -- all of its children serve the purpose a declarative DSL for describing JavaScript calls to the gmaps API.

How will projects like these implement post-render behavior for these inner-components while having no DOM elements?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Looking at ember-google-maps code, there is exactly one use of didRender, which is for the overlay components. This does seem like a valid use case, but based on this and the code in ember-leaflet I don't believe it is that very common as is, and it is not the case we should be optimizing for, even for element-less components.

In cases where users do need this functionality, such as the overlay, I think it would be perfectly reasonable to use a custom component manager to add a didRender hook. This could even be distributed as a render-detector component, like the one being used in ember-google-maps.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I actually think this would be a better suited job for a ResizeObserver actually, the main thing they want is to detect when the content's size has changed, not when it has rerendered.

components. It will run when the element is inserted, and then it will run
again when any of its _args_ change.
* `{{did-render}}` will _not_ render when sub-elements or contents of the
element change. If users need to react to changes in content, they should pass

Choose a reason for hiding this comment

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

This again throws a wrench in the "zero DOM components" story. These components may absolutely want some sort of "my children just changed" hook that doesn't require new attrs to be passed in from the outside (i.e., maybe inner components are changing their own state and re-rendering due to the google maps API returning some data)

Copy link
Owner Author

Choose a reason for hiding this comment

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

In looking at ember-leaflet and the way it works internally, I don't believe that is the case. It seems to manage most of its own state, and actually implements its own hooks. I've addressed this locally and added an example which reproduces ember-leaflet in a minimal fashion, will be pushing shortly.

edition will be completely overhauling the guides and updating all of the best
practices as they stand. New users should see Glimmer components as the
_default_, and should not ever have to write a classic component or see one in
the main guides.

Choose a reason for hiding this comment

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

New users should see Glimmer components as the default, and should not ever have to write a classic component or see one in the main guides.

This would seem to have within it a promise that 100% of Ember.Component use cases should be possible with GlimmerComponent, while staying within the bounds of "octane programming model" best practices.

I have not seen this promise implied or spelled out until here. If we intend this to be a thing, let's call it out.


This may break the policy of "don't talk about deprecating things until the deprecation RFC". I've seen some people advocate for "soft deprecation" via deliberate de-emphasis in docs, and have concerns about whether this sets our community (many of whom will still be working with Ember.Component) up for success

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think we can promise 99% of use cases being covered. The 1% case are cases where class components do things which are strictly more powerful, such as the "render-detector" use case, but these are very rare cases from what we've seen and could generally be solved in other ways (the only truly generic need to detect any render was for positioning using a JS API, and ResizeObserver would have been a better solution).

I don't think we're talking about deprecating classic components just yet, it will be a long time before they are deprecated. But this is a massive overhaul and rebranding effort, and we need to emphasize the new programming model. There will be sections on classic usage, and they will be maintained for the forseeable future.

the work that would otherwise be done in the `constructor` or `init`

* Expense of class fields - new objects and functions are created for every
instance, so users should also be careful with them.
Copy link

@mike-north mike-north Dec 12, 2018

Choose a reason for hiding this comment

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

This section needs a very explicit discussion of how to compare the models of Ember.Object.extend({}) vs class extends EmberObject {}, as it pertains to

  • What ends up on the prototype and what's on the instance
  • How do property initializers work
  • What's the default behavior of a constructor, as it pertains to super() and passing arguments
  • The risks of anonymous classes and class factories (i.e, you get poor stack traces)
  • How to implement "default values" in ES6
  • The risks of writing decorators in user-land code (until TC39 stage 4)
  • Methods vs arrow function member values
  • Getting ahold of prototypes if/when you need them

not as simple as overriding a class field or property. We should guide users to
use template-only components to "curry" components when trying to provide
defaults in subclasses instead, leveraging the outer HTML semantics of
Glimmer components:

Choose a reason for hiding this comment

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

In the current programming model, I'd consider these to be "advanced" techniques. Does the new programming model require that the community level up to use these same techniques for more basic needs?

Copy link
Owner Author

Choose a reason for hiding this comment

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

There are alternatives, absolutely, and we should discuss them. Specifically, using defaulting aliases on the component class, and overriding the defaults in subclasses.

This solution is the "cleanest" one to me, and from what I understand will also be the most performant one at scale since TO components incur so little overhead. It does require us to add ...arguments to make it truly ergonomic, and we're definitely punting on that until after Octane.

But does add a bit of boilerplate to components. Users will also have to be
careful when attempting to override these "defaults" in subclasses, since it is
not as simple as overriding a class field or property. We should guide users to
use template-only components to "curry" components when trying to provide

Choose a reason for hiding this comment

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

I think you mean partial application instead of currying here. Partial application describes the technique of "pass some arguments now and some later", where currying is a specific type of partial application that describes passing exactly one argument at a time.

But does add a bit of boilerplate to components. Users will also have to be
careful when attempting to override these "defaults" in subclasses, since it is
not as simple as overriding a class field or property. We should guide users to
use template-only components to "curry" components when trying to provide

Choose a reason for hiding this comment

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

Ember.js currently allows for some significant choice along the spectrum between

  • "logic-light templates" - derived state is calculated in components and surfaced very simply in templates
  • "logic-heavy templates" - much higher degree of complexity in templates -- up to the point where an Ember app almost feels like a Lisp program that has some JS tossed in for async stuff where necessary

An example of today's logic-heavy templates from the ember-composable-helpers README

{{#each (map-by "fullName" users) as |fullName|}}
  <input type="text" value={{fullName}} onchange={{action (mut newName)}}>
  <button {{action (pipe updateFullName saveUser) newName}}>
    Update and save {{fullName}} to {{newName}}
  </button>
{{/each}}

where the developer is literally defining function pipelines to invoke a series of actions in order.

This choice is largely subjective, and has some non-trivial trade-offs (i.e., debugging tools around templates are comparatively worse and there's no possibility of type safety today, but there's the potenial of future GlimmerVM optimizations optimizing hbs more with AoT stuff). When choosing Ember as a core part of Yahoo's tech stack, we looked very favorably on the idea of templates just being simple HTML that designers could make sense of (not all teams aim for, or achieve this).

I would like to see us carefuly and deliberately avoid narrowing the degree of choice in this area unless it's the purpose of this RFC to do so (in which case, it should be stated directly). As it stands, this RFC (and the accompanying rendering modifiers RFC) feels like a not too subtle effort to push the community toward "heavy" templates.

Copy link
Owner Author

Choose a reason for hiding this comment

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

This RFC does encourage more template usage in general, from the baseline, but there is still a large amount of choice in what users can do and I don't think it's right to characterize it as pushing a "template-heavy" approach.

Yes, templates will be larger with these changes (that's a given due to outer HTML semantics). And yes, there will be a few more template concepts such as the modifiers that users will be using. This is not a slippery-slope toward writing all app logic within templates though. Users who prefer lighter templates will absolutely still be able to place the majority of their logic within classes.

The changes this RFC requires that make templates heavier are:

  1. Outer HTML
  2. Render element modifiers

That is not that much. It is a finite amount more per template/component, not a call to convert the whole thing. The changes this RFC recommends that make templates heavier are:

  1. Using {{@args}} wherever possible/appropriate
  2. Using patterns such as partial application in templates for providing defaults

These are suggestions on how to best use the new features, and again are not calls to push all app/business logic into the template.

components, meaning that for the forseeable future Ember users will likely
need to learn how to use both interchangeably. This introduces a fair amount of
mental overhead for users, but the benefits of Glimmer components and their
simplicity should make this less problematic.

Choose a reason for hiding this comment

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

Suggested change
simplicity should make this less problematic.
relative simplicity compared to `Ember.Component` should make this less problematic.

I'm not convinced that it's obvious which way the balance will lean while we support both programming models, and would like to see any claims of "the benefits will outweigh the costs" statements expressed very clearly as opinions of the RFC author (or core team), instead of being presented as facts.

This would also mean a fair amount of additional code would need to be added for
reacting to changes in the DOM compared to `{{did-render}}` and
`{{will-destroy}}`. For instance, if the case of conditionally captured element,
additional validation code will have to exist in `didRender`:

Choose a reason for hiding this comment

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

The way this is currently framed, it sounds like the "additional code" and validation stuff is an actual realized cost. Really what I think what you mean to say is something like

The opportunity cost of the status quo (conventional lifecycle hooks) is that we miss out on some benefits we could have had, were we using element modifier hooks.

We don't gain complexity or additional code with conventional lifecycle hooks, we keep the similar challeges/benefits that exist around Ember.Component hooks (including not needing to force everyone to make a conceptual adjustment)

```

This problem is compounded in collections, where any number of elements may be
added or removed.

Choose a reason for hiding this comment

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

Again, to be more objective here, you could say

Relative to Ember.Component, there's no improvement around handling collections, where we can see the problem is compounded...


We could alternatively include an `init` hook, or have both. This would allow
users to follow one rule for object initialization, but would also lock us into
the supporting the `init` hook for the forseeable future.

Choose a reason for hiding this comment

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

Qualitatively, what's cost of supporting init for the foreseeable future? Is it high? Isn't this just something we have to include as post-constructor logic around component instantiation?

If we can pay a tiny price and take steps to reduce divergence between things like Ember.Route and glimmer components, we should really consider paying it.

Recent changes to the way native classes extend from `EmberObject` made it so
users have to use `init` instead of the `constructor`. This is a pretty
universal caveat currently, so it's fairly teachable - there is a `constructor`,
but use `init` instead.

Choose a reason for hiding this comment

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

Cite RFC 337

@pzuraq pzuraq force-pushed the glimmer-components branch from 873e7ce to 0f2f02c Compare December 14, 2018 01:27
@pzuraq pzuraq force-pushed the glimmer-components branch from 0f2f02c to 775389c Compare December 14, 2018 02:05
@pzuraq pzuraq merged commit 5466980 into master Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants