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

Conversation

@pzuraq
Copy link
Contributor

pzuraq commented Dec 5, 2018

Rendered

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

@Turbo87
Copy link
Member

Turbo87 left a comment

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

This comment has been minimized.

@Turbo87

Turbo87 Dec 6, 2018

Member

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.

This comment has been minimized.

@pzuraq

pzuraq Dec 6, 2018

Author Contributor

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.

This comment has been minimized.

@Turbo87

Turbo87 Dec 6, 2018

Member

@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

This comment has been minimized.

@pzuraq

pzuraq Dec 6, 2018

Author Contributor

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.

This comment has been minimized.

@jonnii

jonnii Dec 6, 2018

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.

This comment has been minimized.

@frank06

frank06 Dec 7, 2018

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

This comment has been minimized.

@rwjblue

rwjblue Dec 8, 2018

Member

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

This comment has been minimized.

@frank06

frank06 Dec 8, 2018

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.

This comment has been minimized.

@pzuraq

pzuraq Dec 8, 2018

Author Contributor

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.

This comment has been minimized.

@mike-north

mike-north Dec 11, 2018

Contributor

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;

This comment has been minimized.

@Turbo87

Turbo87 Dec 6, 2018

Member

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.

This comment has been minimized.

@tomdale

tomdale Dec 6, 2018

Member

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.

This comment has been minimized.

@jamescdavis

jamescdavis Dec 6, 2018

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.

This comment has been minimized.

@pzuraq

pzuraq Dec 6, 2018

Author Contributor

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];
}

This comment has been minimized.

@Turbo87

Turbo87 Dec 6, 2018

Member

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

This comment has been minimized.

@kanongil

kanongil Dec 10, 2018

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.

This comment has been minimized.

@kanongil

kanongil Dec 10, 2018

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.

This comment has been minimized.

@pzuraq

pzuraq Dec 10, 2018

Author Contributor

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?

This comment has been minimized.

@tomdale

tomdale Dec 10, 2018

Member

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.

This comment has been minimized.

@mike-north

mike-north Dec 11, 2018

Contributor

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;
Show resolved Hide resolved text/0000-tracked-properties.md
```

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

This comment has been minimized.

@Turbo87

Turbo87 Dec 6, 2018

Member

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

This comment has been minimized.

@tomdale

tomdale Dec 6, 2018

Member

@Turbo87 Strong +1 to this suggestion.

This comment has been minimized.

@Turbo87

Turbo87 Dec 6, 2018

Member

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

This comment has been minimized.

@NullVoxPopuli

NullVoxPopuli Dec 6, 2018

Contributor

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

This comment has been minimized.

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

This comment has been minimized.

Copy link

jamescdavis commented Dec 6, 2018

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

This comment has been minimized.

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;

This comment has been minimized.

@kanongil

kanongil Dec 6, 2018

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

This comment has been minimized.

@tomdale

tomdale Dec 6, 2018

Member

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.

This comment has been minimized.

@kanongil

kanongil Dec 7, 2018

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?

This comment has been minimized.

@pzuraq

pzuraq Dec 7, 2018

Author Contributor

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

This comment has been minimized.

@buschtoens

buschtoens Dec 6, 2018

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);
  },
});

This comment has been minimized.

@mixonic

mixonic Dec 6, 2018

Member

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?

This comment has been minimized.

@pzuraq

pzuraq Dec 6, 2018

Author Contributor

@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;
      });
  },
});

This comment has been minimized.

@mixonic

mixonic Dec 6, 2018

Member

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?

This comment has been minimized.

@pzuraq

pzuraq Dec 6, 2018

Author Contributor

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

@jessica-jordan jessica-jordan referenced this pull request Dec 6, 2018

Merged

The Ember Times - Issue No. 76 #3714

7 of 13 tasks complete
@Alonski

This comment has been minimized.

Copy link
Member

Alonski commented Dec 7, 2018

Show resolved Hide resolved 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
constructor() {
this.timer.onTick(() => {
// invalidate the timer field.
this.timer = this.timer;

This comment has been minimized.

@mike-north

mike-north Dec 11, 2018

Contributor

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;
Show resolved Hide resolved text/0000-tracked-properties.md
Show resolved Hide resolved text/0000-tracked-properties.md Outdated
@mike-north

This comment has been minimized.

Copy link
Contributor

mike-north commented Dec 11, 2018

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

This comment has been minimized.

Copy link
Contributor

NullVoxPopuli commented Dec 11, 2018

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link

lolmaus commented Dec 28, 2018

@pzuraq Thank you for a prompt and detailed answer! 🙇

there are quite a few questions that would need to be answered for memoization, including:

  1. What should the decorator be called? @cached? @memo? etc.
  2. Should memoized values trigger change notifications if the new value is the same as the previous value?
  3. Should @memo apply tracking to a property, or should it need to be used in addition to @Tracked? If so, what is the behavior of @memo if used on a non-tracked property?

And there will likely be more questions as part of the design process

Well, can you provide just one implementation that mimics Ember.computed for starters? This drastically reduces the amount of bikeshedding needed.

Then we could migrate from Ember.computed to @tracked/@memo in a straightforward way, without the frustration of not understanding why two usage patterns exist and whether it's okay for them to coexist in one file.

If we were told "just replace @computed with @memo imported from [here] and you're good to go" -- then we could simply convert in a single sitting, forget about @computed and then eventually start exploring different caching strategies.

Note that releasing a drop-in replacement for Ember.computed does not prevent the team from bikeshedding over a better alternative. It doesn't even have to be a single alternative. And thanks to @tracked not being part of the old EmberObject world, generic JS libraries can be used, right? So no bikeshedding or RFCs will probably be necessary! Maybe just have a few suggestions in guides.

@pzuraq

This comment has been minimized.

Copy link
Contributor Author

pzuraq commented Dec 29, 2018

Well, can you provide just one implementation that mimics Ember.computed for starters? This drastically reduces the amount of bikeshedding needed.

Doing this would mean precluding potentially better designs though. For instance, the fact that computed properties swallow notifications if their value didn't change sometimes confuses people, because the property did recalculate and they expect anything dependent on the computed to recalculate as well. I'm not sure where I stand on this, but I think ideally we wouldn't choose a design simply to match the previous implementation. Likewise, I don't think it's best to rush the design for these reasons.

Note that releasing a drop-in replacement for Ember.computed does not prevent the team from bikeshedding over a better alternative

Personally, I believe it does actually. Once a memo decorator becomes part of Ember's public API, it won't make sense to add a second one with slightly different behavior and split the community. It would be better to accept the papercuts of the memo decorator, and provide a solid, clear, conventional path for users.

FWIW, you can continue to use @computed exactly the way you're describing. If you use @tracked properties everywhere else, you won't even need to list property dependencies:

class Person {
  @tracked firstName;
  @tracked lastName;

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

This comment has been minimized.

Copy link

lolmaus commented Dec 29, 2018

@pzuraq Thank you for clarifications. I was almost convinced but then I thought of the following case:

you can continue to use @computed exactly the way you're describing. If you use @tracked properties everywhere else, you won't even need to list property dependencies

What about this example?

class Person {
  @tracked firstName;
  @tracked lastName;

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

  @computed
  get slug() {
    return underscore(this.fullName);
  }
}

Will slug recompute and rerender automatically as firstName changes, given its dependency keys aren't provided?

If not, is it possible to do @tracked @computed get slug(){} to avoid the necessity in dependency keys?

If yes, do you find @tracked @computed reasonable and not a potential source of frustration for regular users? Note that it's quite confusing that sometimes dependency keys are required and sometimes not.

@pzuraq

This comment has been minimized.

Copy link
Contributor Author

pzuraq commented Dec 30, 2018

Will slug recompute and rerender automatically as firstName changes, given its dependency keys aren't provided? If not, is it possible to do @Tracked @computed get slug(){} to avoid the necessity in dependency keys?

It will not, and it is not possible to mark a property as both @tracked and @computed based on this proposal. I do think this is something we could try to close the gap on in followup RFCs, but it will be tricky since @computed still relies on chains and eager observation. If we could deprecate those parts of computed properties first, that would make it much easier.

In the end, I don't think this is a perfect solution, just that it should be good enough for people who need caching in the interim period while we figure out the behavior of @memo. I still think caching is a big enough problem that it needs to be tackled on its own, but there does seem to be enough concern about it here that we can prioritize drafting an RFC for it. We can even try to submit it in parallel with @tracked, as we have with the other Octane RFCs, if that would address your concerns @lolmaus

@lolmaus

This comment has been minimized.

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

This comment has been minimized.

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 tomdale:tracked-properties branch from d68ea65 to 6612cdf Jan 9, 2019

@pzuraq pzuraq force-pushed the tomdale:tracked-properties branch from fb39c14 to c89e5a3 Jan 12, 2019

@rwjblue
Copy link
Member

rwjblue left a comment

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

@Kerrick

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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.

@Panman8201

This comment has been minimized.

Copy link

Panman8201 commented Jan 31, 2019

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

This comment has been minimized.

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lolmaus

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

NullVoxPopuli commented Feb 7, 2019

@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

This comment has been minimized.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment