ember-learn / guides-source

This repository contains the Ember.js Guides
https://guides.emberjs.com/
MIT License
158 stars 497 forks source link

what should we teach in introducing Ember Data Models? computeds vs. decorators #554

Closed ghost closed 5 years ago

ghost commented 5 years ago

We teach computed properties when introducing people to Ember data models. In the Octane Edition we introduce decorators, and later tracked properties. This raises a broader question when refactoring the canonical user model fullName example . I think this question needs answered before question 2.

  1. What should we teach in this example? Computed's, tracked properties?
  2. What should this introductory snippet be refactored to?
MelSumner commented 5 years ago

@igorT @dgeb and maybe @pzuraq would you please weigh in here?

pzuraq commented 5 years ago

I believe we should use Tracked Properties here, and in general. Since tracked props interop with computeds, attrs and relationships should Just Work ™️:

import DS from 'ember-data';
import { computed } from '@ember/object';

export default DS.Model.extend({
  firstName: DS.attr(),
  lastName: DS.attr(),

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

There are two things we need to resolve here:

  1. Can we teach extending from DS.Model using native class syntax? I believe the answer is yes, but I'd like the Ember Data team to weigh in here. The biggest concerns would be around init/constructor, and how those two would interop, since we are trying in general to teach native classes as constructor only (this is part of why classic components are not being taught with native classes).

  2. What is the final syntax for defining a native getter on a classic class? We debated this the week before last, I need to get the above syntax benchmarked and if it looks alright, we could move ahead with it. Otherwise we need to introduce descriptor.

runspired commented 5 years ago

There are a couple of direction we can go from here, I'm going to lay them out and then argue that a particular direction seems best to me:

1. minimal upgrade of syntax

Keeping in mind that changes to the modeling layer are coming, we do the minimal syntactical upgrade and do no rework any documentation further at this time.

2. drop teaching of computed on model

While the modeling future of ember-data is still fuzzy, certain principles exposed by experimentation with ember-m3 as well as exploration of proper architectural separation of concerns when it comes to managing state have taught us that storing derived state and ui state on records is often non-ideal. Additionally, computed properties calculating off of async relationships is difficult-to-impossible to get right. While it is certainly possible to define a DS.Model based class with computed properties on it, we are not convinced that this is a good pattern, and see strong evidence of a future in which helper functions are encouraged to be used instead.

A minimal thing we could do to begin migrating users towards more "functional" instead of OOP patterns for data management would be to stop teaching use of computed properties in this way. It would be a fairly minor drop, and computed properties are well covered elsewhere. I do not believe given the examples of derived state given for components and helpers that we need to teach derived state when teaching models. A simplification here would help folks understand the mental model a bit better and keep it more focused. E.g. I recommend replacing the fullName example with nothing, a pure elimination.

3. leave alone until custom Record Classes lands

@igorT and myself have been working on RFCs to lay the ground work and soon expose the ability for users to provide custom classes (e.g. do not inherit DS.Model) for their records. These RFCs are anticipated prior to Ember Conf which is just a few weeks away. These RFCs will give us the ability to provide a much simpler alternative default model class to our users should we choose, and perhaps we should focus our effort there.

4. full elimination of non-API properties

This is an extension of option 2 but in this case we not only drop use of computed but we additionally refactor the guides to teach functional patterns for deriving state. Some examples of that live in the ember-m3 readme. Consider this the active encouragement alternative to the passive encouragement offered in option 2. The benefit here is users will begin to align their apps to patterns better suited for glimmer components, and better suited for future versions of ember-data in which our default story for data-modeling is closer to that provided by ember-m3.


My personal thinking is currently to go for option 2 in the near term, as it is the minimal and least controversial change, with either an RFC or more extended discussion for option 4 once custom record-classes RFC lands.


A final note in response to @pzuraq:

Can we teach extending from DS.Model using native class syntax? I believe the answer is yes, but I'd like the Ember Data team to weigh in here. The biggest concerns would be around init/constructor, and how those two would interop, since we are trying in general to teach native classes as constructor only (this is part of why classic components are not being taught with native classes).

I think this is a different discussion. We do not want to encourage use of either init or constructor methods for records for similar reasons as wanting to encourage more functional patterns in general.

That said, I believe the brief answer here is that if using something extending DS.Model you should use init, while for custom classes using the constructor will be explored in the custom class RFC. I believe we did some work to make doing work in the constructor "just work" in DS.Model as well, but would have to double check.

ghost commented 5 years ago

@pzuraq @runspired thank you for the detailed responses! I'll defer to core teams consensus here. My pedestrian response follows.

I believe we should use Tracked Properties here, and in general. Since tracked props interop with computeds, attrs and relationships should Just Work ™️:

I see the value to using tracked properties throughout these examples as it meshes nicely with the Octane way of thinking. But I agree with runspired's 2nd point for those reasons. Additionally, I think computeds are an ember idiom that I've acclimated to. Newcomers don't need to know this extra layer to understand modeling data with ember-data. I also think pzuraq's addition of the state management section, and the task for a dedicated section on computeds confirms this example does not need to address computeds.

Can we teach extending from DS.Model using native class syntax? I believe the answer is yes, but I'd like the Ember Data team to weigh in here.


I think this is a different discussion.

It is my understanding that we convert all ember-data examples in the octane branch to use native classes per #543. #553 represents my current understanding: https://github.com/ember-learn/guides-source/issues/543 Perhaps we should continue further discussion in discord or other medium to clarify the correct direction?

runspired commented 5 years ago

@efx the part I meant is a different discussion discussion was the bit surrounding use of init vs constructor. Absolutely we are ready to teach folks to use native class syntax for models.

Much like Ember vs Glimmer Components, DS.Model has provided certain guarantees within init that may not fully translate to a world with constructor usage: however if they don't yet we can get that fixed quickly. This is a case where if we don't yet support (which I believe we do) we can fix rapidly.

ghost commented 5 years ago

ah, that clarifies it; thanks @runspired.

ghost commented 5 years ago

Closing as addressed in #553. (https://github.com/ember-learn/guides-source/blame/octane/guides/release/models/defining-models.md#L49)