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

Tracked Properties #410

Merged
merged 8 commits into from
Feb 5, 2019
Merged

Tracked Properties #410

merged 8 commits into from
Feb 5, 2019

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Dec 5, 2018

Rendered

Big thanks to @wycats, @tomdale, and @chadhietala!

Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

I have one question left: on what kinds of classes are tracked properties supported? only on components? all EmberObjects? all ES6 classes?

in general this looks like a valuable pattern even outside of Ember/Glimmer, and I'm wondering if it's possible to use this on regular classes too

const tracked: PropertyDecorator;
```

This new function will be exported from `@glimmer/tracking`. Revisiting our
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer if we could reexport from an @ember module. It is already hard to remember from where to import things, but adding an additional top-level module scope makes it even harder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reasoning here is to keep @tracked compatible with both Glimmer.js and Ember. This way, users will be able to write component libraries which are compatible with both.

We're working on another RFC at the moment which proposes adding GlimmerComponents to Ember, hopefully this will be submitted by the end of the week. It goes into more detail about this cross-compatibility.

Copy link
Member

Choose a reason for hiding this comment

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

@pzuraq that's why I would propose reexporting so that it works with both paths. the Ember docs can then focus on e.g. @ember/tracking and people that want/need compat can know that both are supported

Copy link
Contributor Author

@pzuraq pzuraq Dec 6, 2018

Choose a reason for hiding this comment

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

That could be a possibility. We're proposing in the Glimmer Component RFC that @glimmer/component be a package independent of Ember.js and Glimmer.js, so the @glimmer namespace would be introduced then as well. I don't think a re-export makes as much sense for Glimmer Components, but @tracked does not collide with any existing concepts in Ember so it could work.

Copy link

Choose a reason for hiding this comment

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

I came to post exactly this. Even as a long time ember user I found this very surprising and I think that @Turbo87's solution to re-export is a good compromise and will make this easier to teach.

Copy link

@frank06 frank06 Dec 7, 2018

Choose a reason for hiding this comment

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

I vote for the re-export. Just as editors can auto-import, they can search & replace. I would definitely make it easier for the Ember developer (i.e. not having to explain what @glimmer is), rather than the pure Glimmer developer. I'm sure they are way less in number

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, this isn't a new thing. Using things like import { DEBUG } from '@glimmer/env' is already public API and somewhat common (more so amongst addon authors I'd suspect).

Copy link

@frank06 frank06 Dec 8, 2018

Choose a reason for hiding this comment

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

I will always vote for reducing the cognitive load on developers new to Ember, no matter how small. This is key for adoption, which we can agree Ember struggles with. If that glimmer API is most common among addon authors, then i'm fine with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Importing from many different namespaces is not uncommon in other frameworks, in fact it’s much more the norm in the wider JS community. One could argue having to learn that there are two possible import paths, and you should use one if you “only care about Ember” and the other if you want cross compatibility, will cause more cognitive load since it deviates from that norm.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would love to see the Ember project take a clear and consistent position on this, as it becomes a lot more of a sore spot once type information is involved.

Another example of the same issue is the Transition type that some route hooks receive (a router.js concept that's exposed in Ember apps). Leaking one library's API through another makes questions like "what's the public API?" more difficult to answer.

constructor() {
this.timer.onTick(() => {
// invalidate the timer field.
this.timer = this.timer;
Copy link
Member

Choose a reason for hiding this comment

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

this is very confusing. I understand what it does, but to anyone unfamiliar, this will look like a no-op that can be deleted. I would prefer it if we could find a more descriptive way to achieve the same thing.

Copy link
Member

Choose a reason for hiding this comment

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

Note that this is the semantic equivalent to this.setState({ timer }) in React to trigger a revalidation. We have some ideas for how we might make this more explicit in the future (as well as potentially eke out some small performance optimizations) but wanted to decouple that from this RFC.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @Turbo87 that, while this makes sense when you understand how tracked properties work under the hood, it will confuse many Ember users. +1 for a more explicit way to mark a tracked property as dirty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Long term, I definitely agree. I actually think we should expose Glimmer's references and validators APIs in a way that allows libraries to intelligently instrument themselves, more so than what we have with tracked properties.

That may take a while though. We bikeshedded for quite a while on some intermediate solutions, including integrating with notifyPropertyChange (avoided so tracked props aren't dependent on Ember) or something like it, but landed on this because it is the minimal proposal. It works because of the nature of tracked properties, and it would work even with alternatives. It allows us to spend some time figuring out what a better alternative would be.

Also, FWIW, a very simple notify function implementable in user-land would be:

function notify(obj, key) {
  obj[key] = obj[key];
}

Copy link
Member

Choose a reason for hiding this comment

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

that snippet seems like a good solution to me if we can import it like import { notify } from '@ember/tracking';

Choose a reason for hiding this comment

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

Currently, whenever this.arr is set, the tracked property bumps a number counter. This is much cheaper in almost all situations, probably one of the cheapest things we can do in the VM. While it's behavior may seem a bit confusing at first, it also means we can't accidentally do more work than we expected. Each function runs at most once.

This is a nice property of the system but only applies to the actual rendering pass. If used outside rendering, it will re-run the expensive getter. E.g. if the property is used in response to incoming events from a remote server.

As such, I think it works better if tracked properties are modelled as "fast", enabling a cheap test to avoid redundant invalidations.

This should also provide a better developer experience, when doing performance optimisations. An expensive getter should show up in profiles, and could often be fixed by applying local caching. This is much nicer than diagnosing the unbounded redundant invalidations the proposed system contains.

Note that this change also makes it more compatible with the listed motivations.

Copy link

@kanongil kanongil Dec 10, 2018

Choose a reason for hiding this comment

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

Actually, the "we can't accidentally do more work than we expected" assertion doesn't even hold for glimmer usage. Say another @tracked property was added like this:

@tracked
get currentTimeString() {
  return `${this.timer.minutes}:${this.timer.seconds}`;
}

Here the timer getter would be called 2 times, though it doesn't change in-between. This illustrates that the proposed system requires careful consumption of properties to avoid inefficiencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think something I haven't really communicated very well here is that there is no silver bullet when it comes to performance. In some cases, it is cheaper overall to rerun getters slightly more often than to hold onto large amounts of memory by eagerly caching everything. In others, caching is definitely more performant, because the work done by the getters is fairly expensive.

Currently, with computed properties, we don't have any granularity around this in Ember. Everything is cached, and users can't opt out of that behavior. We proposed @tracked as a simpler change tracking layer partially to enable that choice, and partially because we believe it optimizes for the common case (relatively cheap getters).

One additional feature we want to add is an @memo decorator, which would allow users to opt back into caching:

@memo
@tracked
get fullName() {}

We decided not to include it in this RFC because it opens a lot of room around bikeshedding the behavior (should it be closer to computed? Should it always trigger setters? Should it require both decorators?) and because we were hoping to build it when the underlying primitives were made public, but it would be pretty simple to build and we wanted to propose it in the near future.

@kanongil do you think this additional decorator would address your concerns? And if so, do you think it should be added to this proposal?

Copy link
Member

Choose a reason for hiding this comment

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

This comment thread has a few sub-threads going now, and I think each of them merits its own discussion and thought. But, while the concerns raised are real, I want to reiterate how rare this situation will be in the vast majority of apps.

Remember: this only comes up when interacting with objects from third-party libraries, where those objects are mutated by the library over time and you want to rely on those properties directly from your templates.

This is actually not supported at all in Ember today. Why? Because if you use an external POJO in a template or computed property, Ember will install the "mandatory setter" that causes an exception to be thrown when the library mutates its own object.

In practice, this ends up being pretty rare in app code. Reflecting changes from a third-party library into something your UI framework understands (whether it's setState, Ember.set or something else) is a problem ecosystem-wide, and we've developed strategies for dealing with it. Usually there will be an addon that adapts a popular library to the idioms of your particular library/framework, which is likely to still be true here.

Because of how widespread this problem is, it also means that most libraries expose callback APIs that pass changed properties as arguments, allowing you to reflect the most up-to-date state onto your component directly.

Even though the RFC shows a case where you can directly utilize mutable, untracked properties on a POJO, I think a more common factoring will be to simply copy properties to the component as they change:

export default class TimerComponent extends Component {
  timer = new Timer();

  @tracked seconds;
  @tracked minutes;

  constructor() {
    this.timer.onTick(({ seconds, minutes }) => {
      this.seconds = seconds;
      this.minutes = minutes;
    });
  }
}

In this example, the timer's onTick callback passes the updated seconds and minutes values. Not only is this factoring less confusing (avoiding the ambiguous this.timer = this.timer), it's also less code overall. 😁

Given the rarity of encountering this situation, and given that there are clear ways of refactoring to avoid the problem, I personally don't think it rises to the level of being worth introducing and teaching an entirely new API.

Copy link
Contributor

Choose a reason for hiding this comment

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

Minimizing WTFs is at least as important as minimizing API surface imo. Even something like

invalidate(this, 'timer');

would convey significantly more developer intent than

this.timer = this.timer;

text/0000-tracked-properties.md Show resolved Hide resolved
```

Now, `reloadUser()` must be called explicitly, rather than being run implicitly
as a side-effect of consuming `fullName`.
Copy link
Member

Choose a reason for hiding this comment

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

we should consider added a dedicated lint rule to eslint-plugin-ember that detects .then() and await in tracked property getters

Copy link
Member

Choose a reason for hiding this comment

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

@Turbo87 Strong +1 to this suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

thinking about it, we should probably have the same for regular computed properties too... 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

there is a use case for this type of behavior though: https://github.com/NullVoxPopuli/ember-computed-promise-monitor

async relationships, for example.
(though, I'm currently of the opinion that they should be avoided at all costs, due to their complexity in accessing the properties on the async relationships)

@pzuraq
Copy link
Contributor Author

pzuraq commented Dec 6, 2018

@Turbo87 to answer your larger question: @tracked will work with all native classes OOTB, and we should definitely add this to the RFC!

Actually reading the validations, however, is a different story. References and tags are still private, and there is no public API yet. So, Glimmer will be the only thing that can read and see if a property has been invalidated. Eventually this should definitely be made public, I could see @tracked being a package that is fully independent of Glimmer/Ember and usable in any context.

@jamescdavis
Copy link
Contributor

I would assume that tracked getters that depend on other tracked getters would work like chained computed properties do (the invalidation would propagate through the chain), but this is not explicitly mentioned in the RFC. e.g. given

export default class Person {
  @tracked firstName = 'Chris';
  @tracked lastName = 'Garrett';

  @tracked get fullName() {
    return `${this.firstName} ${this.lastName}`;
  }

  @tracked get greeting() {
    return `Hello, ${this.fullName}!`;
}

Will a change to firstName trigger GlimmerVM to update the DOM if only greeting is used in the template?

I think the first paragraph under Autotracking describes this when discussing the autotrack stack, but it's not very clear. It might be good to mention chaining tracked getters explicitly and provide an example.

@lifeart
Copy link

lifeart commented Dec 6, 2018

how changes from

computed('users.@each.name', function() {
  return this.users.isAny('name', 'Hodor');
});

can be detected using tracking?
always invalidate users array?

like

changeName(user, name) {
  user.name = name;
  this.users = this.users.slice(0);
}

It's looks very unnatural.

Maybe it's time to force immutability usage in ember?

constructor() {
this.timer.onTick(() => {
// invalidate the timer field.
this.timer = this.timer;
Copy link

Choose a reason for hiding this comment

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

Won't this invalidate the @tracked currentMinutes() ~59 times more than necessary? This seems rather excessive – are there any mechanisms to limit that impact?

Copy link
Member

Choose a reason for hiding this comment

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

Glimmer’s architecture is designed to make this very, very cheap. I’d estimate the performance impact of this to be under 1ms per invalidation on almost every device—probably not something you could detect when profiling.

Copy link

Choose a reason for hiding this comment

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

I'm confused. In #410 (comment) @pzuraq wants to limit re-renders, so it can't really be as inconsequential as you suggest?

People will likely make code like this, where it makes a significant difference. Eg. a 10.000 element array where a single element is changed every frame. Hence my question on how this can be mitigated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kanongil what I meant in that comment was that we would be rerendering the entire app in every render. This would be really expensive, but an individual tracked property is much much cheaper, which is why we want to avoid everything being volatile by default. Within Ember apps, this is likely not going to be an issue. If you updated a 10,000 item array that is rendered, Glimmer will intelligently only update the items that changed within that array. If you had a tracked property based on that array, it was going to rerun anyways, and that code is the responsibility of the user.

However, it is definitely true that users could experience issues if they do not have granularity when trying to wrap external libraries. For now, I think that ideally they would wrap the external library and use its hooks to get as granular as they can. In the future, I do think we should add more granular, low-level APIs which allow users to instrument external libraries more thoroughly.

FWIW, this situation is not much different than where we are today with get/set, notifyPropertyChange, and computed properties. Wrapping external libraries is typically calling notifyPropertyChange on a computed from the libraries hooks, so the perf should be similar.


In order to prevent this from happening, user's will have to use `get` when
accessing any values which may be set with `set`, and are not computed
properties.
Copy link
Contributor

@buschtoens buschtoens Dec 6, 2018

Choose a reason for hiding this comment

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

Maybe add a note here that the easy way out to avoid get and set in this situation is to refactor and mark polling as @tracked. It should also be highlighted that this then requires that the polling property is re-set and not a nested key of it.

const Config = Service.extend({
  polling: tracked({
    value: {
      shouldPoll: false,
      pollInterval: -1,
    }
  }),

  init() {
    this._super(...arguments);

    fetch('config/api/url')
      .then(r => r.json())
      .then(polling => this.polling = polling);
  },
});

Copy link
Member

Choose a reason for hiding this comment

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

Can this be confirmed? Given:

const Config = Service.extend({
  polling: tracked({
    value: {
      shouldPoll: false,
      pollInterval: -1,
    }
  }),

  init() {
    this._super(...arguments);

    fetch('config/api/url')
      .then(r => r.json())
      .then(polling => this.polling = polling);
  },
});
class SomeComponent extends Component {
  @service config;
  @tracked
  get pollInterval() {
    let { shouldPoll, pollInterval } = this.config.polling;
     return shouldPoll ? pollInterval : -1;
  }
}

It has not always been true in the history of tracked properties that pollInterval is invalidated when this.polling = is called on the service. If this works it implies cross-object tracked properties.

@pzuraq is that intended here? Can be possibly be made more explicit in the RFC? I don't think there is an example in this doc for cross-object tracked properties, but maybe I missed it?

Copy link
Contributor Author

@pzuraq pzuraq Dec 6, 2018

Choose a reason for hiding this comment

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

@mixonic can you define what you mean by "cross-object tracked properties"?

To be clear, what this example would be doing is installing the polling tracked property on the Config service. When the polling property is accessed by the component, its tag is entangled with the pollInterval tracked property, so when polling is set, everything is invalidated. The individual properties on polling are not tracked, so this would not work:

const Config = Service.extend({
  polling: tracked({
    value: {
      shouldPoll: false,
      pollInterval: -1,
    }
  }),

  init() {
    this._super(...arguments);

    fetch('config/api/url')
      .then(r => r.json())
      .then(({ shouldPoll, pollInterval }) => {
        this.polling.shouldPoll = shouldPoll;
        this.polling.pollInterval = pollInterval;
      });
  },
});

Copy link
Member

Choose a reason for hiding this comment

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

I think you have described what I mean! :-) There are two objects (the service and the component) and if a tracked property on one (the service) is accessed from a tracked property on the component then they are entangled. Updates to the property on the service will dirty the property on the component.

Do I have it right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! If that's not clear we should definitely make it clear.

More specifically, any tracked getter or property that is accessed from a tracked context (e.g. when an autotrack stack exists) will become entangled with the stack. Templates, tracked getters, and computed properties all start an autotrack stack I believe, so anything that is accessed while those are running will be pushed on.

@tomdale tomdale self-assigned this Dec 6, 2018
@tomdale tomdale added T-framework RFCs that impact the ember.js library Octane labels Dec 6, 2018
@Alonski
Copy link
Member

Alonski commented Dec 7, 2018

@pzuraq The rendered should probably point to here: https://github.com/emberjs/rfcs/blob/be351b059f08ac0fe709bc7697860d5064717a7f/text/0000-tracked-properties.md :)

text/0000-tracked-properties.md Show resolved Hide resolved
text/0000-tracked-properties.md Outdated Show resolved Hide resolved
text/0000-tracked-properties.md Outdated Show resolved Hide resolved
text/0000-tracked-properties.md Outdated Show resolved Hide resolved
text/0000-tracked-properties.md Outdated Show resolved Hide resolved
text/0000-tracked-properties.md Outdated Show resolved Hide resolved
text/0000-tracked-properties.md Outdated Show resolved Hide resolved
constructor() {
this.timer.onTick(() => {
// invalidate the timer field.
this.timer = this.timer;
Copy link
Contributor

Choose a reason for hiding this comment

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

Minimizing WTFs is at least as important as minimizing API surface imo. Even something like

invalidate(this, 'timer');

would convey significantly more developer intent than

this.timer = this.timer;

text/0000-tracked-properties.md Show resolved Hide resolved
text/0000-tracked-properties.md Outdated Show resolved Hide resolved
@mike-north
Copy link
Contributor

It's worth noting that this proposal does not include any patterns for reusable units of derived state (some equivalent to computed property macros) in the context of @tracked. There's almost certainly a need for this, and we should probably get out ahead of it before the community is tempted to do undesirable things like writing their own decorators or ES6-style mixins

// this is pretty much as bad as a mixin
const alteredClass = Base => class extends Base {
	@tracked get foo() { ... }
}

@NullVoxPopuli
Copy link
Contributor

we should probably get out ahead of it before the community is tempted to do undesirable things

while I understand reducing footguns is good, idk if including them in the RFC is able to be done in a concise (or even remotely completish) way. People can get really creative with misuse of language features. :-\

@mike-north
Copy link
Contributor

mike-north commented Dec 11, 2018

while I understand reducing footguns is good, idk if including them in the RFC is able to be done in a concise (or even remotely completish) way.

This is already a very heavy RFC as-is. My suggestion is that if/when it's accepted, a "reusable derived state" RFC should quickly follow.

@pzuraq
Copy link
Contributor Author

pzuraq commented Dec 11, 2018

I don't believe that Ember should necessarily provide an equivalent to computed property macros with tracked properties, specifically because I think this is a wider JavaScript ecosystem concern. With decorators, it should be entirely possible to make a completely Ember independent macro framework, which would work without any alteration with tracked properties:

import { and, or } from 'cool-macro-decorators';
import { tracked } from '@glimmer/tracking';

export default class Demo {
  @tracked first = true;
  @tracked second = false;

  @tracked @and('first', 'second') both;
  @tracked @or('first', 'second') either;
}

The macro library would have absolutely no knowledge of tracking or Ember, it would just add a native getter function to the decorated fields. @tracked would then wrap that getter function with its tracking logic (and this could be coalesced by a bridge library). Ideally, this wrapping would be order independent.

@lolmaus
Copy link

lolmaus commented Jan 9, 2019

@pzuraq Update observers, add unresolved questions d68ea65

This RFC now gives me (an enthusiastic Ember user) an uncomfortable feeling.

Please don't take this as an insult and let me explain. I really, really appreciate the work put into this RFC (and into the general movement toward vanilla JS practices) and I'm not intending to diminish the effort. But I would like to express my concern.

I've adopted Ember because of its bulletproof workflows: use get/set everywhere, don't forget providing dependency keys to CPs -- and you're good to go. It just works, and it beats every other frontend framework. Ember has also had lots of useful tools, and even though some of them have bad rep (mixins, observers, unknownProperty...), I've been able to feel the borderline and used all of them smartly and efficiently.

The new emerging usage style does not seem that robust. We'll be recommended to access properties directly without get/set, but there are gotchas. An app may misbehave unpredictably because some addon is relying on a technique that does not respect @tracked.

Those techniques (observers, unknown properties, proxies, dynamic CP dependencies...) are all gonna be discouraged but still remain legit and non-deprecated.

As a result, I'll have to ask my self the same question every day: do I wanna code this in the clean fancy vanilla JS style and potentially have issues, or should I revert to good old but largely out of fashion techniques (get/set/computed) to stay on the safe side.

And every new app will have a mixture of both styles. Just imagine if it were the case for closure/non-closure properties or modular/global classes. What a mess it would be, both in terms of maintenance and learning curve.

I believe it shouldn't be like that. We should think of a solution that can offer a straightforward transition path that lets users fully convert existing apps to new usage patterns. And for new apps, there should be ways to use the new approach exclusively for everything without getting frustrated by mixtures of new and old Ember patterns.

Rookies should be able to learn the new simple patterns and stay blissfully unaware about the old ones: just like we have forgotten globals, non-closure actions, metamorph tags, using arrayContentDidChange to trigger {{#each}} rerenders, etc.

PS My absolute favorite Ember addons are ember-macro-helpers and ember-awesome-macros by @kellyselden and contributors. Among other beautiful things, they offer a generalized, robust solution for dynamic dependency keys. I've had numerous use cases for them and can't accept dynamic keys being considered an exotic or discouraged pattern.

@pzuraq
Copy link
Contributor Author

pzuraq commented Jan 9, 2019

@lolmaus I totally hear your concerns and I understand the discomfort. I spent a lot of time fretting about the exact same issues throughout the development of this RFC, actually. A few points about them:

  1. We actually entered this uncanny valley before this RFC, with the acceptance of the ES Getters RFC. With that RFC, most of the time it was possible for users to stop using get, but unknownProperty and setUnknownProperty still required get usage. Addons such as ember-m3 and others that make use of proxies have this problem already today, and it hasn't been completely debilitating.

  2. We are unfortunately simply not able to solve some of these problems without new browser APIs, which means dropping support for IE11. Unfortunately IE11 will still be supported by Microsoft until 2025, and while I do think the usage statistics will probably drop low enough sooner rather than later, there's just no way to know when this will happen.

  3. Even if we waited, we will have this uncanny valley for addons no matter what when tracked fields do finally roll out. Ember will not just switch out one model for the other overnight, and addon authors will have to upgrade incrementally no matter what. If they were previously relying on external users watching non-CP properties which are then set with set (a rare but not non-existent use case), then consumers of the addon will have to use get and set to be safe until the addon has updated to use tracked properties, leading to a period where folks will be forced to ask these questions.

Believe me, throughout this process I have been thinking through the path to getting us fully off of set, and I feel very much the same way that we are in an incomplete state that will be difficult for certain apps that have these requirements. However, I believe that the success of ES Getters shows that these use cases are not all that common, and that most applications (and most new users) will be able to use the happy path here. I also believe that the addon ecosystem will be quick to respond by upgrading to tracked properties where appropriate, or even just converting any non-CPs to CPs (which will make them interop perfectly with tracked props).

In the end, we can wait another year or two to cover a few more use cases, and delay the inevitable churn, or we can begin the adoption phase now and be more prepared once we finally can drop IE11 support. And FWIW, addons can begin experimenting with evergreen builds sooner rather than later for apps that don't need to target older browsers, so we can be even more prepared.

@pzuraq pzuraq force-pushed the tracked-properties branch from d68ea65 to 6612cdf Compare January 9, 2019 16:59
@pzuraq pzuraq force-pushed the tracked-properties branch from fb39c14 to c89e5a3 Compare January 12, 2019 00:53
Copy link
Member

@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.

We discussed this at the core team meeting today, and we believe that this RFC is ready to move into final comment period.

@Kerrick
Copy link

Kerrick commented Jan 30, 2019

Unrelated to the suggestions in this RFC but related to its text... Can anybody explain this to me? Where can I read more about these changes coming in Octane? I had no idea that we were getting rid of computed properties, classic components, event listeners, etc.

The classic programming model refers to the traditional Ember programming model. It includes classic classes, computed properties, event listeners, observers, property notifications, and classic components, and more generally refers to features that will not be central to Ember Octane.

@lolmaus
Copy link

lolmaus commented Jan 31, 2019

@Kerrick, @lifeart has pointed me at this document by @wycats which seems to be a roadmap draft.

It has been created all the way back in 2015! But it looks like in the very beginning of 2019 @wycats has updated it to match decisions made around this RFC.

That roadmap resolves part of my frustration around this RFC: tracked properties seem to fit nicely into a consistent object model.

What's lacking, unfortunately, is a smooth transition path from the "classic" to the new object model. This seems to be impossible. 😿 Which in turn means that old and new object models will coexist and the old model is unlikely to ever be deprecated without violating Ember's commitment to stability. 😕

@pzuraq
Copy link
Contributor Author

pzuraq commented Jan 31, 2019

@Kerrick to be clear, nothing is being removed in Octane feature-wise. No deprecations have been written for any of the classic object model, and it will take quite some time for them to be removed even if they are deprecated in the future.

What that refers to is exactly what it says - the older concepts will become less central. I think the best precedent here is observers - they still function, and we have no plans to remove them, but they are less recommended when compared to computed properties or other methods for watching changes. In the same way, Octane will recommend some newer features over older ones:

  • Glimmer components over Classic components, for outer HTML and general performance
  • Tracked properties over computed properties, for simpler change tracking
  • Angle bracket invocation over curly bracket invocation, for clearer and cleaner templates
  • Lifecycle hooks over using on()

The older features will still exist, still be fully documented, and still be discoverable. They just won't be recommended in general.

@pzuraq
Copy link
Contributor Author

pzuraq commented Jan 31, 2019

@lolmaus I'm not entirely sure I agree. Consider this component taken from Ember Observer:

import { inject as service } from '@ember/service';
import Component from '@ember/component';
import { computed } from '@ember/object';
import { isEmpty } from '@ember/utils';
import { task } from 'ember-concurrency';
import { filterByFilePath } from '../utils';

export default Component.extend({
  visibleUsageCount: 25,

  showUsages: false,

  usages: null,

  regex: false,

  fileFilter: null,

  codeSearch: service(),

  visibleUsages: computed('visibleUsageCount', 'usages', function() {
    return this.usages.slice(0, this.visibleUsageCount);
  }),

  moreUsages: computed('visibleUsageCount', 'usages', function() {
    return this.visibleUsageCount < this.usages.length;
  }),

  fetchUsages: task(function* () {
    let usages = yield this.codeSearch.usages.perform(this.addon.id, this.query, this.regex);
    this.set('usages', filterByFilePath(usages, this.fileFilter));
  }).drop(),

  actions: {
    toggleUsages() {
      this.toggleProperty('showUsages');
      if (this.showUsages && this.usages === null) {
        this.fetchUsages.perform();
      }
    },

    viewMore() {
      let newUsageCount = this.visibleUsageCount + 25;
      this.set('visibleUsageCount', newUsageCount);
    }
  }
});

function filterByFilePath(usages, filterTerm) {
  if (isEmpty(filterTerm)) {
    return usages;
  }
  let filterRegex;
  try {
    filterRegex = new RegExp(filterTerm);
  } catch(e) {
    return [];
  }
  return usages.filter((usage) => {
    return usage.filename.match(filterRegex);
  });
}

First we can convert it to native classes with the native class codemod

import { inject as service } from '@ember/service';
import Component from '@ember/component';
import { action, wrapComputed, computed } from '@ember/object';
import { isEmpty } from '@ember/utils';
import { task } from 'ember-concurrency';
import { filterByFilePath } from '../utils';

export default class AddonSourceUsagesComponent extends Component {
  visibleUsageCount = 25;
  showUsages = false;
  usages = null;
  regex = false;
  fileFilter = null;

  @service codeSearch;

  @computed('visibleUsageCount', 'usages')
  get visibleUsages() {
    return this.usages.slice(0, this.visibleUsageCount);
  }

  @computed('visibleUsageCount', 'usages')
  get moreUsages() {
    return this.visibleUsageCount < this.usages.length;
  }

  @(task(function* () {
    let usages = yield this.codeSearch.usages.perform(this.addon.id, this.query, this.regex);
    this.set('usages', filterByFilePath(usages, this.fileFilter));
  }).drop())
  fetchUsages;

  @action
  toggleUsages() {
    this.toggleProperty('showUsages');
    if (this.showUsages && this.usages === null) {
      this.fetchUsages.perform();
    }
  }

  @action
  viewMore() {
    let newUsageCount = this.visibleUsageCount + 25;
    this.set('visibleUsageCount', newUsageCount);
  }
}

Next, we can convert it to tracked properties. Crucially, we can do this one property at a time, since computed properties and get/set fully interoperate with tracked properties:

import { inject as service } from '@ember/service';
import Component from '@ember/component';
import { action, wrapComputed, computed } from '@ember/object';
import { isEmpty } from '@ember/utils';
import { task } from 'ember-concurrency';
import { filterByFilePath } from '../utils';

export default class AddonSourceUsagesComponent extends Component {
  @tracked visibleUsageCount = 25;
  @tracked showUsages = false;

  usages = null;
  regex = false;
  fileFilter = null;

  @service codeSearch;

  @computed('usages')
  get visibleUsages() {
    return this.usages.slice(0, this.visibleUsageCount);
  }

  @computed('usages')
  get moreUsages() {
    return this.visibleUsageCount < this.usages.length;
  }

  @(task(function* () {
    let usages = yield this.codeSearch.usages.perform(this.addon.id, this.query, this.regex);
    this.set('usages', filterByFilePath(usages, this.fileFilter));
  }).drop())
  fetchUsages;

  @action
  toggleUsages() {
    this.showUsages = !this.showUsages;

    if (this.showUsages && this.usages === null) {
      this.fetchUsages.perform();
    }
  }

  @action
  viewMore() {
    this.visibleUsageCount += 25;
  }
}

And then fully converted:

import { inject as service } from '@ember/service';
import Component from '@ember/component';
import { action, wrapComputed, computed } from '@ember/object';
import { isEmpty } from '@ember/utils';
import { task } from 'ember-concurrency';
import { filterByFilePath } from '../utils';

export default class AddonSourceUsagesComponent extends Component {
  @tracked visibleUsageCount = 25;
  @tracked showUsages = false;
  @tracked usages = null;

  regex = false;
  fileFilter = null;

  @service codeSearch;

  get visibleUsages() {
    return this.usages.slice(0, this.visibleUsageCount);
  }

  get moreUsages() {
    return this.visibleUsageCount < this.usages.length;
  }

  @(task(function* () {
    let usages = yield this.codeSearch.usages.perform(this.addon.id, this.query, this.regex);
    this.usages = filterByFilePath(usages, this.fileFilter);
  }).drop())
  fetchUsages;

  @action
  toggleUsages() {
    this.showUsages = !this.showUsages;

    if (this.showUsages && this.usages === null) {
      this.fetchUsages.perform();
    }
  }

  @action
  viewMore() {
    this.visibleUsageCount += 25;
  }
}

Then we can convert to Glimmer components:

import { inject as service } from '@ember/service';
import Component from '@glimmer/component';
import { action, computed } from '@ember/object';
import { isEmpty } from '@ember/utils';
import { task } from 'ember-concurrency';
import { filterByFilePath } from '../utils';

export default class AddonSourceUsagesComponent extends Component {
  @tracked visibleUsageCount = 25;
  @tracked showUsages = false;
  @tracked usages = null;

  @service codeSearch;

  get visibleUsages() {
    return this.usages.slice(0, this.visibleUsageCount);
  }

  get moreUsages() {
    return this.visibleUsageCount < this.usages.length;
  }

  @(task(function* () {
    let usages = yield this.codeSearch.usages.perform(
      this.args.addon.id, 
      this.args.query, 
      this.args.regex
    );
    this.usages = filterByFilePath(usages, this.args.fileFilter);
  }).drop())
  fetchUsages;

  @action
  toggleUsages() {
    this.showUsages = !this.showUsages;

    if (this.showUsages && this.usages === null) {
      this.fetchUsages.perform();
    }
  }

  @action
  viewMore() {
    this.visibleUsageCount += 25;
  }
}

These steps could happen in any order, and are very flexible. As we discussed before, there will be cases where users have to continue using get/set as they convert and as the community converts, and unfortunately this will likely be necessary until we can drop IE11 for certain use cases (proxies), but this is not worse than today with native getters.

So, given that, I think that we have a very interoperable new set of features that blends with the older features, and will allow us to gradually replace them over time. There will not be a need for a hard deprecation, or breaking changes that violate Ember's core value of stability without stagnation.

@Panman82
Copy link

FWIW, here are the diffs for all those steps;

  1. Native classes codemod: https://www.diffchecker.com/7oxJZCSa
  2. Tracked properties: https://www.diffchecker.com/Fvxpd4ab
  3. Fully converted: https://www.diffchecker.com/rTk0GpE5
  4. Glimmer component: https://www.diffchecker.com/Ru7vUpWA

@rwjblue
Copy link
Member

rwjblue commented Feb 5, 2019

Thanks to everyone for there diligence in reviewing and thinking about this RFC! Lets do it! 🎉

@rwjblue rwjblue merged commit 0798670 into emberjs:master Feb 5, 2019
@lolmaus
Copy link

lolmaus commented Feb 7, 2019

I've got a question about lack of observers.

Imagine a simple editable label component. It displays text, but when clicked, the label changes into an input field.

With two-way data-binding it would look like this:

<EditableLabel @label={{this.myText}}/>

With one-way data-binding it would look like this:

<EditableLabel
  @label={{this.myText}}
  @onRename=(action (mut this.myText))
/>

The user input is now stored in some private field like _label which is passed into the onRename action when user finishes editing.

But additionally I want to control the edit mode externally, so it now looks like this:

<EditableLabel
  @label={{this.myText}}
  @isEditing={{this.isEditing}}
  @onRename=(action "updateBothLabelAndIsEditing")
  @onCancel=(action (mut this.isEdtiting) false)
/>

Now I have proceeded to edit the label by setting isEditing in the parent template to true. I have edited the text and thus mutated _label.

After that, I have cancelled edtiting by setting isEditing in the parent template to false.

When I proceed to editing again, I will see that my previous changes have persisted. That's obviously not desired. When I restart editing, the input field should display the current state of the parent value and not the user input from the previous cancelled attempt.

If I had observers, I'd simply do this:

@observer('isEditing')
clearOnCancel() {
  if (!this.isEditing) {
    this.set('_label', '');
  }
}

But how in the world can I clear the field without observers?

I can think of two ways:

  1. Move the internal _label property from the component to parent controller/selector.
  2. Pass an ad-hoc event bus EmberObject.extend(Evented) into the component and trigger an event on the bus whenever the user cancels editing from the parent scope.

Both solutions are obviously ugly, if not to say worse.

Is there a better one?

CC @simonihmig

@NullVoxPopuli
Copy link
Contributor

@lolmaus, I'd just do this:

init() {
  super.init();

  this.value = this.args.label;
}

@tracked _isEditing = false; // internally used is editing flag
@tracked value = '';

@tracked
get isEditing() {
  if (this.isExternallyManaged) {
    return this.args.isEditing;
  }

  return this._isEditing;
}

@tracked
get isExternallyManaged() {
  return this.args.isEditing !== undefined;
}

onRename() {
  this.args.onRename(this.value);
  this._isEditing = false;
}

onCancel() {
  if (this.isExternallyManaged) {
    return this.args.onCancel(/* args or something */);
  }

  this._isEditing = false;
}

or something like that :)

@pzuraq
Copy link
Contributor Author

pzuraq commented Feb 7, 2019

While as @NullVoxPopuli pointed out, it should generally be possible to refactor to avoid observers, we actually changed that part of the RFC back and forth a few times. In the last iteration, I had a discussion with @krisselden where he outlined a path forward that would allow observers to work completely within the tag-based system that power's @tracked, the only caveat being that the timing semantics of observers would change slightly.

This is why the current text states that we will attempt to make tracked properties and observers work together, and are pretty confident we should be able to do so. I don't believe this will be ready for the Octane preview for EmberConf, but hopefully we'll be able to complete it sometime after.

@2hu12
Copy link

2hu12 commented Oct 22, 2019

how changes from

computed('users.@each.name', function() {
  return this.users.isAny('name', 'Hodor');
});

can be detected using tracking?
always invalidate users array?

like

changeName(user, name) {
  user.name = name;
  this.users = this.users.slice(0);
}

It's looks very unnatural.

Maybe it's time to force immutability usage in ember?

Do we have an answer for this? cc @lifeart

@lifeart
Copy link

lifeart commented Oct 22, 2019

@2hu12 user object must have tracked @name.
and native implementation for hodor user should looks like:

get hodor() {
  return this.users.find(({name})=>name === 'Hodor'));
}

@lifeart
Copy link

lifeart commented Oct 22, 2019

@lolmaus
Copy link

lolmaus commented Oct 22, 2019

@lifeart What is the object is just a POJO? Classic Ember was able to track that, IMO.

@NullVoxPopuli
Copy link
Contributor

Classic ember could only track things on objects extending from EmberObject.

Now, with actual native POJOs, you have two options:
Invalidate the reference, or use a class with tracked properties within the class

@lolmaus
Copy link

lolmaus commented Oct 22, 2019

Classic ember could only track things on objects extending from EmberObject.

@NullVoxPopuli Uhm, that's not true? Classic Ember would automatically wrap a POJO and properly track changes on it. Please see this Twiddle: https://ember-twiddle.com/?numColumns=2&openFiles=templates.application.hbs%2Ccontrollers.application.js

Not sure how that changes with Octane.


you have two options: Invalidate the reference

When displaying large, deeply nested data structures with a hierarchy of components, invalidating the reference would cause everything to rerender, when only a single deeply nested property actually changed.


or use a class with tracked properties within the class

That's impossible with dynamic schema, and a performance penalty with static schema.

@NullVoxPopuli
Copy link
Contributor

cause everything to rerender

It shouldn't because of the Glimmer VM / "Virtual DOM".

Not every set is rendered.

@pzuraq
Copy link
Contributor Author

pzuraq commented Oct 22, 2019

@lolmaus classic Ember was able to track mutations to POJOs because every set and get was intercepted via Ember.set and Ember.get. These still work with autotracking today, as outlined in this RFC, and will continue to be part of the Octane programming model, so if you need to dynamically instrument certain properties, that's an option.

Alternatively, we can continue to explore other options. This is why I created tracked-built-ins, which explores both options that use native Proxy to instrument objects dynamically, as well as decorators. It's built on public API, so it's also an option (I would recommend using it without Proxy for now, due to performance caveats).

@NullVoxPopuli is also correct here in that it won't cause a full rerender since Glimmer does diff the final output of each value in the template, but it would still cause the entire DOM tree to revalidate which is not ideal either. I do think that some apps will need to ultimately update objects/arrays directly for performance. The options for doing this boil down to:

  1. Create a class with explicit tracked properties (for objects only)
  2. Use get/set for objects and KVO compliant methods for arrays (pushObject, etc). We do need to finish up the array functions RFC so these can be used with plain native arrays.
  3. Use a library like tracked-built-ins

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.