Deprecate component lifecycle hook arguments.

[chancancode]

From me and @locks, based on the recent Ember Core Team F2F discussion.

Rendered

Ember PR: emberjs/ember.js#14711

[tomdale]

Decisions like this are always tricky. In this case, we were passing arguments that we considered to be private, but there wasn't a good way to signal that in code itself. It's pretty common for users to look at the arguments passed to a hook to see what information they have to work with.

In almost all cases, I'd be in favor of maintaining those APIs, even if they were less than perfect, just to avoid the perception of churn.

In this case, though, because they have a non-trivial performance impact on every app, and the actual functionality that incurs the cost is so infrequently used, I think it's a rare situation where we can use the fact that they were not documented as a justification for their deprecation.

[workmanw]

This makes me little 😢 . I feel that lifecycle hooks were in part touted as the observer killer. Instead of observing for a value change [to take some action], you just implement this hook. For that to remain the case, I think we need a good dry way to know what was actually changed.

I'm okay with sacrificing the ability to know what the old values are (I can always keep track of them myself), but if there is a way to at least know what values were changed, that would be very helpful. Without that, it quickly goes back to manually caching all values, comparing for differences, then taking the action(s). We use the args enough in our app that it feels like common boilerplate. I guess I could always just implement a mixin for this work, but wanted to provide the feedback nonetheless.

[buschtoens]

I feel like this is semi-related, so I'm just gonna ask: what about this.attrs?

Is it officially supported yet? If not, when? And when will attrs stop over-shadowing component properties?

[chancancode]

@workmanw do you mind sharing a few examples/snippets from your app?

[chancancode]

@buschtoens definitely not related at all 😉 I want to keep this RFC narrowly scoped to the private arguments and don't want to open that other can of worms here; maybe another time 😄

[workmanw]

@chancancode Sure! It usually boils down to one of two case.

Querying data from the server. Example:

didReceiveAttrs(options) {
  if (!options.oldAttrs || options.oldAttrs.user.value !== options.newAttrs.user.value) {
    this.fetchUserAuditRecords();
  }
},

fetchUserAuditRecords() {
  let userId = this.get('user.id');
  let store = this.get('store');
  store.queryRecord('audit', { user: userId }).then(auditRecords => {
    if (userId === this.get('user.id')) {
      this.set('auditRecords', auditRecords);
    }
  });
}

Resetting some application state:

didReceiveAttrs(options) {
  // Start out each post with the comments collapsed
  if (!options.oldAttrs || options.oldAttrs.user.value !== options.newAttrs.user.value) {
    this.set('commentsCollapsed', true);
  }
}

In both of these cases I could store the old values and compare them, but this really feels like boilerplate type of work. I just thought that if the performance complication was more about keeping a full set of old and new values, as opposed to just knowing what changed, it could be a great win. Either way, not the end of the world.

[chancancode]

@workmanw just to clarify, are those working examples from your current app, or are you proposing them as a new API that better fits your needs?

[workmanw]

@chancancode Those were meant to be working examples ... but I just wrote them from memory so they were much more simplified and forgot to add the oldAttrs.user.value !== newAttrs.user.value checks. My apologies. I've edited the examples to show what should be (fingers crossed) actual working examples. The sentiment I was trying to convey was avoiding redundant AJAX or some other non-idempotent action (commentsCollapsed) by knowing what has actually changed.

Again, not the end of the world. I can always build a helper mixin to make this easier. Just pointing it out because I think most people who use the options (args) just want to know what changed. Example: TryGhost/Ghost-Admin/.../gh-content-preview-content.js

[rwjblue]

I am generally opposed to changing the semantics of these hooks until they are actually designed. We should have an RFC describing them and their purpose before we add more churn without really knowing the future state.

[bendemboski]

This would also need an

init() {
  this._super(...arguments);
  this.set('_previousCoordinates', this.get('coordinates'));
}

no?

[workmanw]

@bendemboski I thought that when I first read it too. But because didReceiveAttrs is called both on initial render and update render (Guides) it is not necessary.

[bendemboski]

didReceiveAttrs() is, but didUpdateAttrs() isn't, so if you only want to run code when attrs are updated but not when they are initialized (as in this "before" example) and you want to track previous values, I'm pretty sure you need to implement init() also.

[buschtoens]

@chancancode No problemo. :)

Much in the same sentiment as @workmanw I thought that didReceiveAttrs in particular was meant to be the observer killer. For that to work users need to know when specific attrs actually did change, which is currently only possible by manually diffing them which feels needlessly un-dry and cumbersome.

As already pointed out the next logical step would be writing a configureable mixin that does this automatically. IMHO this is something that should not be left up to the user (to possibly screw up 😅 ). This is such a central and fundamental problem that Ember itself should at least offer this mixin or a suitable alternative.

---

Also, there's another problem that I feel like there's no adequate solution for - @chancancode, please scold me, if this is unrelated / out of scope again. 😆

@rwjblue mentioned that these hooks aren't actually fully designed yet (?), so this should be taken into consideration.

(Sorry for the long text, there's a TL;DR at the bottom.)

When writing components in a truely DDAU fashion you strictly couple child components to their parent component's state and don't allow deviations down the line, which is necessary in some edge-cases.

What do I mean by that? The fantastic ember-one-way-controls addon offers "true" strictly coupled DDAU components. Their {{one-way-input}} component always displays the value that was passed into it. When the app user enters new characters or otherwise manipulates the value _processNewValue is called with the new DOM value and in turn invokes the update action, if the DOM value actually differs from the passed in value.

In the next render loop _syncValue which (besides keeping track of the cursor position) sets the DOM value to the actual value that was passed in as an attr. That means, that if the parent component choses to "ignore" the update action or otherwise transforms the value, that very value will be displayed.

There are no deviations down the line. The child component always displays the parent's state and is strictly coupled.

This is the intended behavior and is what a true DDAU component should look like on paper.

However in the field, this strict coupling can cause problems. ember-scrollable, which basically offers pretty scrollbars (demo), a is a good example. There's an onScroll action that can be used for getting the current scroll position and there's a scrollTo attr for setting the scroll position.

DDAU, so far.

If ember-scrollable was to strictly couple the scroll position to the scrollTo attr (as ember-one-way-controls does), you would experience a nightmarish scroll jank. I actually implemented these two features and couldn't find adequate advice on the Ember Community Slack and just went with diffing scrollTo and only ever manually updating the scroll position when the attr changes, so the app user can scroll freely in the meanwhile.

And now (finally) to the actual problem: What if I set scrollTo to a certain value (0 for instance) by clicking a button, then scroll down a bit and again set scrollTo to that value by clicking the button? You would want the container to scroll back to that position again, but actually scrollTo did not change. So nothing happens.

In my app I currently work around this by setting scrollTo to null (which doesn't trigger any scroll movement by design) and then in the next render loop set scrollTo to that value of interest again.

This is a major pain point for me.

TL;DR: You cannot always strictly couple a child components state to the attrs passed into it by its parent for performance reasons (e.g. scroll jank). Therefore, sometimes, users must allow deviations form the parent state down the line. This is problematic, if the parent then explicitly wants to instruct a child component to assume a certain state that it previously was in. The only interface for communicating down are attrs. If these attrs don't differ, because the parent wants the child to return to that state, nothing happens as the child cannot detect changes.

We'd either need a way to call an action in the opposite direction (down, which doesn't feel good) or somehow explicitly mark attributes as dirty / changed, even though they actually didn't.

[buschtoens]

Also, here's the DiffAttrsMixin we're using.

[dwickern]

typo

[Panman82]

I know this has been mentioned already, but my biggest need from args would be to know which attribute(s) changed.

Why not decide public args instead of depreciating everything? I suppose you need to depreciate before changing...

[buschtoens]

@Panman8201

[...] my biggest need from args would be to know which attribute(s) changed.

You could use the mixin I linked for that.

The motivation behind deprecating these arguments is that they a) were never meant to be public in the first place and b) add a significant performance loss to every Ember app, even though this app might not be using these arguments.

Quote from the RFC:

The reason to officially deprecate lifecycle hook arguments is not only about messaging, but also because providing these arguments imposes an unnecessary performance penalty to every component in your application even if the arguments are not used.

[Panman82]

You could use the mixin I linked for that.

I would think that ember would know exactly which attributes changed and could pass those names to the hook. Wouldn't it be more efficient than comparing old values to new values, and less prone to comparison issues? Just seems like an obvious thing the hook could provide.

[Gaurav0]

This is such a central and fundamental problem that Ember itself should at least offer this mixin or a suitable alternative.

I used to think this way. Once I started to work for enterprises, however, I discovered quickly how much more I dislike API churn. Now I feel that new features should be tried out in addons and then moved to core once we know we aren't going to change anything.

If someone would release the mixin in an addon, and add proper testing, I'd like to try it out in some of my apps. ;)

[grapho]

At first I was opposed to deprecating these.. but I feel like the arguments are pretty strong for removing them.

Give 'em the boot

I do have some very large complex components which might be tricky to know exactly what has changed.. but more and more I find myself (especially now with contextual components) making smaller and smaller components where only one or two attrs are passed in. In these components, it is less important to know which attr changed.

[Gaurav0]

Please note any perf improvements won't be seen until Ember 2.13 at the earliest.

[buschtoens]

making smaller and smaller components where only one or two attrs are passed in. In these components, it is less important to know which attr changed.

So much this. We're currently heavily refactoring our app, which already started way back in Ember 1.x times, and breaking up complex components into many small parts appears to be a a good pattern so far.

This alone often already prevents the cannot set x twice in a single render loop error.

Components, which are becoming the the central part of Ember, should be as lightweight as possible, so that users would rather render many small instead of one big complex component.

[wycats]

I wanted to try to summarize the arguments as I understand them so far.

For

• Performance: The hooks, as currently formulated, intrinsically add cost to every component, whether the arguments are used or not (and even for components that have no JS class at all).
• Accidental "feature": The implementation of the hooks and their signatures never went through an RFC process and leaked in by accident through blog posts. They were never carefully designed, and this thread illustrates that they're not even a good fit for the problems people are trying to solve with them.
• Confusion / Messaging: The hook signatures have become somewhat public (through Stack Overflow, etc.), and failing to clearly mark them as deprecated furthers the confusion about the current state of the components. The current status is also making it more difficult for the highly active community to clearly message its intent to more casual users.
• Experiments outside of core: Things like attribute diffing make sense for Ember, but we have been doing more of these kinds of experiments outside of core. In this case, the problem has a non-trivial design space, and Ember has enough primitives to enable experimentation.

Against

• Observer killer: The hooks were touted (on blog posts) as observer-killers, and this change makes them harder to use.
• Boilerplate / un-DRY: The replacement patterns described in the RFC are very un-DRY and full of boilerplate.
• Which attr changed?: It's important to know which attr changed, not just that something has changed.
• Ember knows more: It would be more efficient to do the work in Ember, since Ember knows exactly which attrs have changed.
• Churn: In general we avoid introducing unnecessary churn in the programming model for our users. This is unnecessary churn.
• Bridge to nowhere: We shouldn't deprecate this feature before we have fully designed the replacement programming model (Glimmer components), because the process of designing Glimmer components may give us alternate paths that introduce less churn.

---

I'll follow-up shortly with my thoughts.

[wycats]

On performance, I want to clarify a few things about my position.

First, because we have to supply the old attrs in the current formulation of the hooks, there isn't any way to make reifying the attributes lazy. By the time we're ready to call the hooks, the old values don't exist anymore in their original slots, so we are forced to save them off, whether you end up needing them or not.

In contrast, this.attrs is much easier to make lazy and pay-as-you-go, because it always refers to the current value of the attributes.

The consequence of being forced to snapshot the old attributes ahead of time is added baseline cost for even the simplest of components.

@grapho said:

more and more I find myself (especially now with contextual components) making smaller and smaller components where only one or two attrs are passed in. In these components, it is less important to know which attr changed.

@buschtoens said:

Components, which are becoming the the central part of Ember, should be as lightweight as possible, so that users would rather render many small instead of one big complex component

I agree with these points. We want small-components to be the Ember programming model, but added mandatory cost for every component makes it harder for people to break things up more. Ironically, as @grapho pointed out, smaller components mitigate the need for such granular attr tracking.

Also worth noting: built-in components like link-to use the hooks but do not rely on the arguments. The status quo adds performance cost to every instance of link-to. Today, it adds performance costs to every components because of the existence of noop hooks on the superclass (which, itself, is not trivial to change without impacting compatibility).

I'll also add that we might be able to reduce some (but certainly not all) of these costs with significant amounts of implementation effort, but the question is whether the community should be pouring countless hours into making this intimate API perform well instead of working on more general optimizations in Glimmer, adding new features like Glimmer components with lower overhead, or working on improving the performance of evaluating the Ember payload.

[wycats]

On the issue of boilerplate, I think this thread illustrates that patterns using the intimate API also have quite a bit of boilerplate (and quite error-prone, as we saw!)

The purpose of the didUpdateAttrs hook (and similar hooks) is to allow users to update the DOM manually (using jQuery, etc.) whenever the inputs change. In general, there are better, more reliable ways to accomplish these kinds of things in Ember (use <div class="{{val}}"> rather than toggleClass in a hook). Occasionally low-level code or interoperating with third-party libraries really does require manually reflecting input values into the DOM.

However, each situation is different: you might want to just === every single attribute to see if it changed, but you might be counting up the number of elements in a list in a single attribute and only updating the DOM when then count changes.

In practice, there are many different strategies that make total sense:

• checking reference equality (===) of all attr
• checking reference equality of a single or subset of attr
• using an id in an attr to decide whether something has changed (this is the strategy #each uses)
• using a more custom structural check in a particular attribute
• reducing one or more attributes down into a value ("count of items", "average age") and only updating the DOM when the reduced value changes

All of these are valid use cases, and people should absolutely experiment with addons (or in their apps) with strategies that efficiently implement the various solutions.

Component.extend({
  didReceiveAttrs: checkEquals('name', () => {
    // update DOM
  })
});

Component.extend({
  didReceiveAttrs: equiv({
    keys: ['name', 'address'],
    equivalence(oldKeys, newKeys) {
      return deepEqual(oldKeys.name, newKeys.name) &&
        oldKeys.address.id === newKeys.address.id;
    },
    hook() {
      // update DOM
    }
  })
});

The nice thing about these solutions is that they are (1) actually more ergonomic and less error-prone for their use-cases than what we have today, (2) opt-in, which means components that don't use equality checks in their hooks don't pay the cost, and (3) implementable in terms of today's Ember!

[wycats]

On the topic of churn, I think it's important to note that churn goes both ways: if we don't signal that a feature without a future has no future, more people will adopt the feature, which will introduce churn for a larger group of people later on.

The question we should be discussing as a community here is whether we think this feature has a future. I strongly believe that the answer is no (and the discussion at the core team face to face reached the same conclusion), and I think we should focus this RFC on that question.

If we can come to agreement that this feature has no future in Ember, we should signal as much to the broader community.

[chancancode]

I agree with @wycats about churn – that fact that we are avoiding to communicate about this clearly is undermining our ability to message other things (e.g. "controllers are fine, you should keep using them – they are not even deprecated today") and our overall credibility. So long as we can agree on the (lack of a) future of this feature, we are not doing anyone any favor in the long run by not communicating that publicly/clearly.

@workmanw @Panman8201 would it help the transition if some of the code snippets here are extracted into an (unofficial) addon?

[workmanw]

@chancancode Naw, I can write my own utils, I like to think that I'm pretty good at that :)

My concern is newer ember users. I worry about the discoverability. This just feels like core functionality to me. I mean heck, an observer will tell you what field change when it fires. As a new user, I wouldn't think to go find an addon to get the same functionality for hooks.

EDIT: Just to kind of explain my view a little more. Over the past few years I've seen Ember more and more opt to not include functionality in the core, but to advocate it be an addon. I definitely understand the reason for that, maintainability (among others). But the hitch with that is the guides/docs never discuss functionality of addons like that. I worry that when new users comes to emberjs.com and follows the guides, unless they feel obligated to jump off and find addons for themselves, it's harder for them to fully realize the value. In this case there is a guide that talks about the hooks. A new user reading that guide may ask "how do I know what attrs have changed?". If this were a core argument it would be documented in that guide. Being the functionality of an addon, it will not.

[ef4]

@workmanw, you raise an important point about discoverability and teaching. I want to make it very clear that any addon that's part of the default ember-cli blueprint is part of the first-class, core experience and the docs and guides are intended to treat them as such. Testing, Fastboot, and Engines are all core pieces of the experience that don't work without addons.

If in the future, all support for routing was broken out into an ember-routing addon, and we put that addon in the default ember-cli blueprint, nothing would have to change from a learning & discoverability perspective. It would still be just as first-class, discoverable, and documented. The only difference is increased flexibility for advanced users who want to mix and match at the next layer down.

For a feature like the one we're discussing here, it's entirely reasonable to argue for including it in the core experience and documenting it in the guides, but even then it would still belong in an addon whenever possible.

[workmanw]

@ef4 Yea, I definitely understand that. My comments were not as much about the technical line between a "first-class addon" (e.g. testing) and the core itself, but rather about a 3rd party addon like this one that comes out of an RFC process. Something that was almost "core / 1st party addon".

Honestly, even if the guides and docs were to suggest/highlight certain 3rd party addons would go a long way. In this case, if there was a small snippet that said, "You can use an addon such as X to track attributes that have changed between calls", I think that would help a lot.

But now I've completely wandered away from this point of this RFC. My apologies for that. I think my concern has been conveyed. Thanks for listening.

EDIT: Just to be clear, I'm not trying to hold up the show here. Definitely seems like this is a go and the impact to me personally is minor. Just trying to highlight what I feel is a regular frustration I hear when I talk to people on the forums, at meetups and even within my own company.

[acorncom]

Honestly, even if the guides and docs were to suggest/highlight certain 3rd party addons would go a long way. In this case, if there was a small snippet that said, "You can use an addon such as X to track attributes that have changed between calls", I think that would help a lot.

@workmanw We've been internally debating this on the learning team over the last 6 months or so and are open to adding those types of links. Would you mind opening an issue with your suggestions or sending in a PR? Our main gotcha has been how well supported the recommended addon would be, but as addons become more and more central to Ember we do a disservice to guides readers if we don't make some key recommendations.

and now back to our regular broadcast 😀

[wycats]

@workmanw I think it makes sense to include functionality like we've been discussing in the core experience, but regardless of what it looks like, it would be opt-in (where the current functionality is a cost every component has to pay).

I want the functionality to be implemented in addons to explore the space more, but I fully expect the results of that exploration to end up as an RFC and eventually included in the core experience.

In the meantime, it seems like there is agreement both that there is no future for the signature under discussion here, and that there is sufficient functionality available in Ember to provide an offramp for people relying on the intimate functionality.

[chancancode]

After discussion at the core team meeting this morning, this RFC is now entering a "final comment period". This period is intended to focus the community's attention for a final round of consideration and feedback before an RFC is merged.

Please carefully consider the implications of this RFC and how it will affect your Ember projects. Now would be a good time to re-raise any concerns that you feel haven't been fully addressed.

If no significant blockers are raised by Friday, December 30th, we plan to merge this RFC and make plans for its implementation.

[workmanw]

@wycats So I took your thoughts, iterated a little bit, and ended up with ember-diff-attrs.

One challenge that presented itself with using a macro approach was didUpdateAttrs. Because we don't have a good way (that I know of) to hook into init as a macro, I couldn't preform necessary setup and thus we're unable to tell what had changed the first time didUpdateAttrs was invoked. You can get the desired functionality by using didReceiveAttrs because it is called on initial render.

That said, the marco solution feels much much better than implementing as a mixin.

All feedback is welcome. Sorry for posting slightly off topic.

[rwjblue]

Thanks everyone for the discussion here. Since it seems like all concerns have been addressed and it has been 7 days since we entered final comment period, this is ready to go.

[GavinJoyce]

For anyone interested, I'm proposing some changes to the ember-diff-attrs API by introducing a new didChangeAttrs hook:

workmanw/ember-diff-attrs#4 (comment)