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

Decorators #408

Merged
merged 8 commits into from
Jan 11, 2019
Merged

Decorators #408

merged 8 commits into from
Jan 11, 2019

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Dec 4, 2018

Rendered

Big thanks to @pzuraq !!!

@tomdale
Copy link
Member

tomdale commented Dec 4, 2018

Awesome work! This looks great.

To me, the biggest risks are bringing back action namespacing collisions and requiring decorators to be invoked (e.g. @computed() instead of @computed). Fortunately, both of these are relatively easy to lint against.

Do we have a sense of whether any popular addons rely on subclassed ComputedProperty functionality?

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

rtablada commented Dec 4, 2018

@NullVoxPopuli one thing of confusion with language is:

Notably, using computed as a decorator directly will not be supported.

This seems a bit confusing since "directly" here isn't super clear for those who aren't quite familiar with the spec or decorator syntax.

I think a bit more explanation of this or moving it to be the first or last example of decorators. The grouping seems odd and a bit of a break and flow adding to some confusion.

When I personally read the sentence I had to do a double take because I was like "wait you're saying I can't use computed but all the examples so far have shown computed.

Maybe this makes more sense in "drawbacks" or just more language around how @computed propName is invalid but @computed() propName is valid.

@pzuraq
Copy link
Contributor

pzuraq commented Dec 4, 2018

Maybe better phrasing here would be:

Notably, computed must be always be called as a function before being used as a decorator. It is not possible to use @computed without parenthesis.

@NullVoxPopuli
Copy link
Contributor Author

Notably, computed must be always be called as a function before being used as a decorator. It is not possible to use @computed without parenthesis until sometime after the old non-decorator macros are removed after the next major version

?

@tomdale
Copy link
Member

tomdale commented Dec 4, 2018

@NullVoxPopuli Generally, my experience is that it's better to avoid making claims about whether some functionality will be deprecated or removed, unless it's a deprecation RFC. Otherwise it's too easy for the RFC discussion to turn into a meta-RFC about removing the other behavior, instead of focusing on the benefits of the new behavior.

@tomdale
Copy link
Member

tomdale commented Jan 4, 2019

We reviewed this RFC at the core team meeting today, and given the new language around decorators support and the TC39 stage system, we have consensus that this design is ready to enter Final Comment Period.

@rwjblue
Copy link
Member

rwjblue commented Jan 11, 2019

Given that this has been in FCP for a week, we reviewed it again at the core team meeting today. The team is still in favor of moving forward, and there have been no additional comments since being moved into FCP: let's do it!

@rwjblue rwjblue merged commit 423a06a into emberjs:master Jan 11, 2019
@mike-north
Copy link
Contributor

This RFC is being made with the assumption that decorators will be moved to stage 3 in the near future, before this RFC is implemented in Ember, dramatically reducing the risk of adopting decorators. If this RFC is accepted and decorators are not advanced in a timely manner, a followup RFC should be made to determine whether or not decorators should be adopted in stage 2, and what the support for them would look like.

Given that last week's TC39 meeting failed to advance decorators to stage 3, a follow-on RFC is probably necessary at this point

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Feb 4, 2019

I think a feature flag would still be reasonable :)
(though, I don't know if that'd be possible).

afaik, the implementation allows the old syntax to work

@pzuraq
Copy link
Contributor

pzuraq commented Feb 4, 2019

There will definitely be a followup RFC addressing this turn of events, along with our plans to mitigate them. Currently we don’t expect this will affect the path moving forward for Octane, as discussed in the RFC, decorators (or something that provides the same functionality) are a pretty hard requirement for us to move forward to native class syntax. This may push back the release of Octane, though we should still be able to preview them by EmberConf.

@sheriffderek
Copy link

I applaud all the thought going into this - and it's because I respect this effort so much, that I feel like I have to give another perspective / even though I don't feel any where near the caliber of folks on this thread.

...there are still key features of Ember that are not usable with class syntax today, including computed properties, actions, and injections.

Without knowing the beast or its belly, I certainly believe and respect all the history here. I did not mind the classic model syntax one bit, but I accept that I don't know enough about it to offer another option. I do wonder, were we bullied into this? Did this appease a portion of the prospective developers only to complicate the situation?

These features cannot be used ergonomically with native classes as it stands. The only options are to either use Ember's defineProperty function directly or to define these values in an anonymous class. Both of these options are hard to read, and will be difficult to codemod in the future:

OK. All of these examples are clear / and not very fun ^. But the only option left as it seems / is also not very fun.

...The actions namespace was added as a convenience to prevent these collisions, and to separate them from the rest of the class body organizationally.

I can understand that this was added as a necessity - but I'd like to bring up that it's very clear and nice to use. It's not unlike the methods: {} hash in other frameworks. Similarly, could a 'computed' hash also solve for scope and the tripple references? I would like to see some code examples here with many actions and computed properties. Vue is what it will be compared to.

...and could be counterintuitive to newcomers:

I think this most definitely would be_ the case. Ember has always been ahead of game - but now that the game is afoot, it might be a good time to think about how abstraction might not be out nessesity - but for user benefit. As a user, I feel like Ember has been getting really really close to having an undeniably clear api - and that this is a few steps away from that. It's highly possible that I haven't seen the light yet / but after thinking about this for the last few days / I can't help but think that this is just super ugly. I'm a diehard / but my team is basically gone already.

...leave it to user's to know which lifecycle hooks exist.

I think that it's fair for a framework to assume that it's users know how it works. I've stumbled my way with Ember over the last 4 years / and not a JS guru - but I never named a function didInsertElement. If trusting the user makes writing the code 4x better - it's a big win.

@pzuraq
Copy link
Contributor

pzuraq commented Feb 27, 2019

@sheriffderek so is your main concern about the lack of an actions hash in native classes, and how that will affect the clarity of class definitions? And that Vue is continuing down the path of having actions along with their computed hash, so everything is well organized?

I think my main concern with following Vue in that way is they don't seem to have a path forward to native class syntax with those types of features. I agree it would be nice to be able to arbitrarily organize and group some categories of things, but at the same time I think betting on native classes to be the future is very good bet. Frameworks will likely start to feel like they're aging when classes really become the standard in JavaScript in general, over the next few years.

There may be ways to add that kind of scoping functionality back in the future with decorators, but we should probably wait and see to be sure.

@sheriffderek
Copy link

sheriffderek commented Feb 27, 2019

@pzuraq I guess my main concern is that this syntax along with tracked properties will scare off anyone interested in Ember. Like I said, I don't know enough about decorators - and have only used them a tiny bit to learn about them in a sandbox. Between this and typescript - it feels like things are always pushing the boundary farther then the average person really cares to go. I'm certainly glad it's getting pushed - but like a runway outfit, it usually gets dialed back for public use. My coworker wrote out a component in a few different frameworks syntaxes and put them next to each other. He kinda sighed and said - "I'm a developer. I don't care what's native syntax... I just want to be able to do my job." I always trust the decision process that you guys make - as far as what is 'best' / but just trying to add a voice that isn't so close to the project's core. It seems like decorators could live in the shadows like generator functions. They seem really powerful - but how we use them is really what matters.

If it's all just a layer on a layer on a layer - of functions. I'm just making a case for whatever is easiest to read - but I know that's subjective. I'll use this CSS preprocessor mixin as an example. Which one would you rather write 100 times a day?

:root {
  --my-mixin: {                
    background: #256dbd;
    color: #f5f5f5;
  }
}
body {
  @apply --my-mixin;
}



my-mixin()
  background: #256dbd
  color: #f5f5f5

body
  my-mixin()



@mixin my-mixin {
  background: #256dbd;
  color: #f5f5f5;
}
body {
  @include my-mixin;
}



my-mixin() {
  background: #256dbd
  color: #f5f5f5
}

body {
  my-mixin()
}

Also, imagine that you're writing 30 of these rules. hundreds of @import in the file. Do we write @action above every action? I'm loving my Octane experience so far, but when I got to using a service I ran into these decorators / and I'm stumped.

Will we be able to opt out?

@NullVoxPopuli
Copy link
Contributor Author

I can't speak for @pzuraq , but here are some of my thoughts (also, thanks for sharing your concerns!):

I guess my main concern is that this syntax along with tracked properties will scare off anyone interested in Ember

from my point of view, I think it'll encourage people to try out ember, because it's a relatively thin layer on top of existing javascript.

"I'm a developer. I don't care what's native syntax... I just want to be able to do my job."

Native syntax has some performance and cross-developer learning benefits.
Native syntax allows people from other ecosystems to pick up on things quicker.

If it's all just a layer on a layer on a layer - of functions

With ESNext*, over time, this is less and less true, cause things are actually getting implemented in browsers / v8 / node. Super exciting! :D

I'll use this CSS preprocessor mixin as an example.

depends on end-user needs.
Personally, I'm wanting to get away from all CSS pre-processors that don't allow me to use native CSS variables, because I want my apps to be themeable in user-space, without javascript.

hundreds of @import in the file.

this is a bit extreme. :)
One of the points of all this is explicitness better conveying intent / where things are coming from so that someone unfamiliar with a project can ctrl+click there way through everything and know exactly where everything is coming from. :)

Do we write @action above every action?

in the future, with glimmer components (the future!)
just using native functions without @action would work.
see the octane blueprint example in the readme: https://github.com/ember-cli/ember-octane-blueprint#glimmer-component-example

I'm loving my Octane experience so far, but when I got to using a service I ran into these decorators / and I'm stumped.

Will we be able to opt out?

Afaik, temporarily.
There are no deprecation RFCs for today's / classic syntax.

The way I see it, when the current stuff is eventually deprecated and removed, the ember gzip payload size is going to get a lot smaller. :D
I'm super excited for that :)

@pzuraq
Copy link
Contributor

pzuraq commented Feb 27, 2019

As @NullVoxPopuli pointed out, classic class syntax will stick around for some time. And as he pointed out, this move really is about removing layers - in the long run, decorators will just be JavaScript. When you switch to a new framework from Ember, they'll use the exact same classes, similar decorators, and lots of the same features - and vice-versa.

That's really the core idea here - every framework ends up reinventing the same core things for classes, computed properties, change tracking, etc. They just do it in slightly different ways. Native classes, and decorators, are a standardization at the language level, so in time, all frameworks and libraries will pick them up 😄 Ember is getting the jump on that by adopting now, but we won't drop classic class syntax any time soon.

Also, I totally understand the pain of having to learn a new syntax/features, we're working on thorough guides to help out with learning and updating. I also understand the pain around having to annotate every function with @action, but there is a converse side to this: You don't have to use {{action}} in templates all the time now, thanks to the autobinding:

export default class MyComponent extends Component {
  @action
  sayHello() {
    console.log('Hello, world!');
  }
}
<button onclick={{this.sayHello}}>Say Hello!</button>

@sheriffderek
Copy link

sheriffderek commented Feb 28, 2019

@NullVoxPopuli & @pzuraq - Thanks for taking the time to answer. I get the rub here - for sure. I've always thought that getting closer to native JS was a good selling point / and reasoning. I've sold it to other developers that way when they were concerned about the extend - but now it's here! I can also see that as a detractor for some. This is probably a pretty accurate portrayal of how people are thinking about choosing things in real life: https://www.youtube.com/watch?v=OsBrp15zTIE - and in that vein, I could imagine them saying - 'yeah that's great, but we can just write a few decorators.' Developer ergonomics isn't something we talk about in the survey. I'll get some kool-aid and work on some realworld use of decorators and tracked properties - and see if I can come around.

Are there any other good examples in the wild besides emberclear? I can learn a lot from that - but the typescript is another layer to understand.

@NullVoxPopuli
Copy link
Contributor Author

@sheriffderek did you read the comments on that video? :)

Developer ergonomics isn't something we talk about in the survey

the ember community survey?
hmm. it's too late to add now, but with the octane's release coming up, it'd probably be a good thing to ask about on the survey next year

I'll get some cool-aid

what do you mean?

Are there any other good examples in the wild besides emberclear? I can learn a lot from that - but the typescript is another layer to understand.

all my octane apps/addons are in TypeScript. :\

@pzuraq do you have any?

@sheriffderek
Copy link

sheriffderek commented Feb 28, 2019

@NullVoxPopuli - RE comments on video: Yes. I read them. I'm not saying that that video has any value besides a window to a less invested 'coders' mindset. I see this attitude at a lot of the JS meetups. RE cool-aid: I'm just saying I'm going to try my best to learn to love these changes.

I can write a webapp with only native JS / (or React or whatever) but then no one on my team would know how to use it. That's why we use Ember. We can count on a shared language. We're training a few Jrs and interns now with 3.7 brackets invocation etc. and it's going really well. I know this is a big shift that needs to happen, but If Ember changes the controls instead of the engine - then it's no longer doing what we chose it for.

I've seen a lot of Ember organization like this: (this is just an example - copy and pasted from a few things to see the buckets a bit - not a working thing)

screen shot 2019-02-28 at 8 28 53 am

So, I'm not saying "Well, Vue does x..." - more than I am curious about how creating a hash for something like actions - or computed - mirrors many people's mental model - and could also take away the need for a lot of the registration and this.get('prop') - type things in computed properties. I see uses in Ember where I don't want a props array like Vue, but it's also something that people can use to clearly see what is expected to come in. Just another example.

I'm curious what the ideal interface would be - if there was nothing holding you back. Maybe decorators is it. Maybe there can be an experiment add-on that extends the class with a hash and - which simply drops @computed in front of every key in that hash. I don't know how that stuff works. I'm just a user. And as a user - this reminds me a lot of when Angular went from 1.5 to 2-3-4 or whatever it's at - and I went all in on Ember. Maybe I'm just scared. I'll work on an example with all the new syntax. A small component - and try and work into it.

@NullVoxPopuli
Copy link
Contributor Author

fwiw, everything is backwards compatible, which is our big advantage over the Angular 1 to 2 transition (at least initially, anyway -- they retroactively added backwards compat).

If I convert your example to my ideal modern syntax, it'd look like this:

interface IArgs {
  info: any;
  questions: any[];
}

// NOTE: @glimmer/component doesn't yet take a type arg, like sparkles-component does
export default class SomeComponent extends Component<IArgs> {
  @storage game;
  @storage settings;

  @service('page-interface') page;
  @service audio;

  @alias('currentStep.stepType') stepType;
  
  // state
  @alias('game.currentIndex') currentIndex;
  @equal('stepType', 'welcome') isStartPage;
  @equal('stepType', 'end') done;
  
  // tracked properties / auto-tracking -- no annotation needed afaik
  get totalQuestions() {
    return this.questions.filterBy('stepType', 'question').length;
  }

  get percentComplete() {
    return this.totalAnswered / this.totalQuestions) * 100;
  }

  // private function, I think this is stage 3 of TC39?
  #exampleFunction() {

  }

  setMenu(menuName) {
    this.page.setMenu(menuName);
  }

  closeMenu() {

  }

  init() {
    super.init(...arguments);
    // init is mostly optional, depending on what you're doing
  }

  didInsert(element) {

  }

  willDestroy() {

  }
}
<div 
  ...attributes
  {{did-insert this.didInsert}} 
  {{will-destroy this.willDestroy}}
>

  <button {{action this.exampleFunction}}>
    click me
  </button>
  
</div>

though, to be fair, I think this component is doing waaay to much and should be broken up a bit.. :-\

but yeah, all that syntax should work today/soon, depending on ember version / canary, I think.

@pzuraq
Copy link
Contributor

pzuraq commented Feb 28, 2019

@sheriffderek I totally understand the hesitation you're feeling to this change, it is a big shift to the framework and I can see why it might feel like the same type of shift as Angular 1-2. This new syntax is foreign, no matter what, and it will take some time to get used to.

I don't want to try to convince you that the syntax is better or not - the reality is, it's mostly an aesthetic difference for end users. The wins in native classes are mostly performance based in the long term, and the Ember Object model is capable of most of the same things as native classes, which is how we're able to continue supporting the object model (funnily enough, the reason this is true is because the Ember Object model was so influential in the design of classes and decorators, so it all comes full circle a bit). We could keep doing this for some time, though I do believe that eventually native classes will start to gain functionality we can't support, like private class fields.

The point is, you like it. New users may find it weird, or prefer native classes, but this syntax has served us well, served you well, so I get why it feels like we may be throwing the baby out with the bathwater.

I want to point out two things that are different about this change than Angular 1-2:

  1. We are not breaking the world all at once. We aren't going to force you to switch your entire application over to native class syntax when you upgrade to the next Ember version. Both syntaxes will work side-by-side, which means you'll be able to convert one component or class at a time. This is the key difference between Ember and other frameworks - upgrading an Ember app is always an incremental process. You don't need to rewrite the whole app, you can take 10 minutes here, 20 minutes there, and get it done piece by piece, in between shipping features and app code.

  2. We are continuing to support classic classes not just to make the transition easier, but also as a hedge. If the community decides that native classes are worse, or less ergonomic, or that classic classes continue to have benefits, then we can always pivot back to them. The core team believes that native classes are the path forward, but isn't going to force the community to accept that.

    I think this is the best way for our community to validate new ideas. Rather than forcing a massive shift, we are supporting people who want to adopt the new syntax, and we'll see what the results are. If native classes are as successful as we think they'll be, then it'll make sense to move forward with them, and that's when the deprecation of classic classes will happen. Not before that.

I hope that helps make you feel better about all these changes. My ask, to you and to the entire community really, is to give the new features and mental model an honest try (when they are released in stable Ember) - convert a few components and classes. Take a look at how they work, and compare your code before and after. We can't debate and design these features in a vacuum, we need to actually try them out in production applications, at scale, to know whether or not they'll be successful.

No matter how certain the core team is, it's entirely possible that we were wrong about things, we're only human after all. This is why we always introduce new features and let them be integrated into the community, battle-tested in production apps, and validated at scale, and then move to remove old features. If the new features don't work in practice, we can walk them back. If the old features still provide value, we can continue to support them. This is how we can achieve stability without stagnation 😄

josemarluedke added a commit to josemarluedke/ember-apollo-client that referenced this pull request Mar 11, 2019
We are going to use the computed decorator as a macro and polyfill to the Ember computed described in emberjs/rfcs#408.
josemarluedke added a commit to josemarluedke/ember-apollo-client that referenced this pull request Mar 11, 2019
This uses `computed` from `@ember-decorators/object`.

This allow us to use computed as a macro in classic ember objects using `EmberOjbect.extend({ ... })` as well as a decorator when using classes.

In the future, `@ember-decorators/object` would only serve us as a polyfill for older Ember versions as this is becoming the default behavior in Ember per [Decorators RFC](emberjs/rfcs#408)
josemarluedke added a commit to josemarluedke/ember-apollo-client that referenced this pull request May 30, 2019
We are going to use the computed decorator as a macro and polyfill to the Ember computed described in emberjs/rfcs#408.
josemarluedke added a commit to josemarluedke/ember-apollo-client that referenced this pull request May 30, 2019
This uses `computed` from `@ember-decorators/object`.

This allow us to use computed as a macro in classic ember objects using `EmberOjbect.extend({ ... })` as well as a decorator when using classes.

In the future, `@ember-decorators/object` would only serve us as a polyfill for older Ember versions as this is becoming the default behavior in Ember per [Decorators RFC](emberjs/rfcs#408)
josemarluedke added a commit to josemarluedke/ember-apollo-client that referenced this pull request Aug 1, 2019
We are going to use the computed decorator as a macro and polyfill to the Ember computed described in emberjs/rfcs#408.
josemarluedke added a commit to josemarluedke/ember-apollo-client that referenced this pull request Aug 1, 2019
This uses `computed` from `@ember-decorators/object`.

This allow us to use computed as a macro in classic ember objects using `EmberOjbect.extend({ ... })` as well as a decorator when using classes.

In the future, `@ember-decorators/object` would only serve us as a polyfill for older Ember versions as this is becoming the default behavior in Ember per [Decorators RFC](emberjs/rfcs#408)
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.

7 participants