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

[QUEST] Allow plain ES6 classes to be services #16134

Closed
4 tasks
wycats opened this issue Jan 16, 2018 · 47 comments
Closed
4 tasks

[QUEST] Allow plain ES6 classes to be services #16134

wycats opened this issue Jan 16, 2018 · 47 comments
Assignees

Comments

@wycats
Copy link
Member

wycats commented Jan 16, 2018

Now that the ES2015 classes RFC has been merged, it's time to take advantage of the internal cleanup to allow Ember applications to use ES2015 classes in their applications.

We're going to start with services because the Service superclass is an empty subclass from Ember.Object.

However, there are still a number of things to work out.

Context and Background

First of all, not all of the functionality of the RFC has yet been implemented. For the most part, the problems are minor and related to .extend()ing an ES2015 subclass of an Ember superclass. Since this quest is about using basic ES2015 classes that do not extend from Ember.Object or Ember.Service, it is not directly relevant, but @pzuraq is working on identifying these limitations and writing failing tests for them.

Second, in order to support simple objects, we will need to decide on the public API for dependency injecting into ES classes, which requires deciding on a stable public API for dependency injection in general. The smallest incremental step is to expect a static create method that takes the owner, but we might want to do a little more than that. I'll take responsibility for writing an RFC.

Finally, we will need to decide how to handle computed properties in these news classes. The most incremental step is to not support computed properties directly in Ember and still require Ember.set to set properties directly on services. The ember-decorators addon could be used to get sugar for those features.

However, if we expect people to feel that they need to use ember-decorators every time they use bare classes, we should probably pull enough into Ember proper to avoid that. For example, we could add a @tracked decorator for fields that (for the moment) calls Ember.set under the hood. We will all need to review our own apps to decide how many services in the real world rely a lot on Ember object model features.

Current Status and Work Items

  • Identify limitations in the current support for ES classes (🔒 @pzuraq)
    • Fix or defer them
  • Write an RFC for dependency injecting outside of old-style Ember classes (🔒 @wycats)
  • Survey real-world apps for use of Ember object model features in services

The most useful thing that you could do right now is go through your app and survey all of your services for the use of certain features:

Total services: <count>
is an object proxy: <count>
injects another service: <count>
alias: <count>
readonly: <count>
non-volatile computed properties: <count>
observers: <count>
concatenated or merged properties: <count>
actions: <count>

For example, this is my survey of Skylight:

Total services: 9
is an object proxy: 1
injects another service: 3
alias: 1
readonly: 1
computed properties (not volatile): 0
observers: 0
concatenated or merged properties: 0
actions: 0

Hang out in Community Slack

We'll be coordinating this work on the community slack. You can join the #st-es-class-services channel.

@wycats wycats self-assigned this Jan 16, 2018
@wycats wycats added the Quest label Jan 16, 2018
@josemarluedke
Copy link
Contributor

josemarluedke commented Jan 17, 2018

Awesome to see this!

Here is my survey. This does not include services provided by add-ons, which is about 6 more services.

Total services: 8
injects another service: 6
is an object proxy: 0
aliases: 1
readonly: 0
non-volatile computed properties: 5
observers: 0
concatenated or merged properties: (0)
actions: 0
EventedMixin: 0

@kumkanillam
Copy link
Contributor

Here is our app output,

Total services: 12
is an object proxy: 0
injects another service: 11
alias: 7
readonly: 0
computed properties (not volatile): 24
observers: 0
concatenated or merged properties: 0
actions: 0```

@betocantu93
Copy link
Contributor

betocantu93 commented Jan 17, 2018

Three production apps surveys:
1.

Total services: 7
is an object proxy: 0
injects another service: 6
alias: 0
readonly: 0
computed properties (not volatile): 1
observers: 1
concatenated or merged properties: 0
actions: 0
Total services: 2
is an object proxy: 0
injects another service: 2
alias: 0
readonly: 0
computed properties (not volatile): 1
observers: 0
concatenated or merged properties: 0
actions: 0
Total services: 5
is an object proxy: 0
injects another service: 3
alias: 0
readonly: 0
computed properties (not volatile): 1
observers: 0
concatenated or merged properties: 0
actions: 0

As @josemarluedke, this does not include services provided by add-ons.

@GavinJoyce
Copy link
Member

GavinJoyce commented Jan 17, 2018

Intercom (numbers are counts of services, eg. 16 services use inject (most multiple times, 30+ in total)):

Total services: 44
is an object proxy: 0
injects another service: 16
alias: 0
readonly: 5
computed properties (not volatile): 20
observers: 0 💪
concatenated or merged properties: 0
actions: 1

@zeppelin
Copy link
Contributor

zeppelin commented Jan 17, 2018

Total services: 10
is an object proxy: 0
injects another service: 6
alias: 1
readonly: 2
computed properties (not volatile): 14
observers: 1
concatenated or merged properties: 0
actions: 4 👈

👉 Actions are techincally just methods, called using the bind helper from templates.

@lukemelia
Copy link
Member

Here is data on 2 of the more complex apps at Yapp:

App 1:

Total services: 18
is an object proxy: 0
injects another service: 7
alias: 0
readonly: 1
non-volatile computed properties: 5
observers: 1
concatenated or merged properties: 0
actions: 0

App 2:

Total services: 26
is an object proxy: 0
injects another service: 10
alias: 0
readonly: 5
non-volatile computed properties: 15
observers: 1
concatenated or merged properties: 0
actions: 0

Also, several services mix in Ember.Evented and have ember-concurrency tasks

@bradleypriest
Copy link
Member

Here's data on our core app at TradeGecko

Total services: 72
is an object proxy: 0
injects another service: 40
alias: 1
readonly: 6
computed properties (not volatile): 23
observers: 1
concatenated or merged properties: 0
actions: 0

As well as several ember-concurrency tasks, and one usage of unknownProperty

@jonnii
Copy link

jonnii commented Jan 17, 2018

One of our internal apps:

Total services: 15
is an object proxy: 0
injects another service: 12
alias: 0
readonly: 0
computed properties (not volatile): 5
observers: 0
concatenated or merged properties: 0
actions: 0

@knownasilya
Copy link
Contributor

App 1

Total services: 10
is an object proxy: 0
injects another service: 4
alias: 1
readonly: 0
computed properties (not volatile): 4
observers: 0
concatenated or merged properties: 0
actions: 0
EventedMixin: 1

@simonihmig
Copy link
Contributor

Total services: 18
is an object proxy: 0
injects another service: 12
alias: 1
readonly: 3
non-volatile computed properties: 13
observers: 2
concatenated or merged properties: 0
actions: 0

not counting addons, numbers are number of services that use one or more of these features, not total count of instances

@jamesarosen
Copy link

Total services: 18 (excluding -dom-tree-construction, -dom-changes, -glimmer-environment, -routing)
is an object proxy: 1 (localStorage)
injects another service: 16
alias: 0
readonly: 3
non-volatile computed properties: 4
observers: 2
concatenated or merged properties: 0
actions: 0

@sandstrom
Copy link
Contributor

Total services: 36
is an object proxy: 2
injects another service: 24
alias: 3
readonly: 0
non-volatile computed properties: 19
observers: 2
concatenated or merged properties: ? [don't know what this is]
actions: 0

@jgwhite
Copy link
Contributor

jgwhite commented Jan 18, 2018

For the Heroku dashboard:

Total services: 22
is an object proxy: 0
injects another service: 12
alias: 4
readonly: 4
non-volatile computed properties: 22
observers: 2
concatenated or merged properties: 0
actions: 0

@jdhorwitz
Copy link

For https://digitalroots.com / https://interactions.com

Total services: 15
is an object proxy: 1
injects another service: 14
alias: 4
readonly: 0
computed properties (not volatile): 73
observers: 1
concatenated or merged properties: 0
actions: 0

@rlivsey
Copy link
Contributor

rlivsey commented Jan 18, 2018

For MinuteBase

Total services: 17

is an object proxy: 0
injects another service: 11
alias: 0
readonly: 2
non-volatile computed properties: 5
observers: 0
concatenated or merged properties: 0
actions: 0

For Kayako

Total services: 70

is an object proxy: 0
injects another service: 51
alias: 0
readonly: 3
non-volatile computed properties: 16
observers: 0
concatenated or merged properties: 0
actions: 0

@pzuraq
Copy link
Contributor

pzuraq commented Jan 18, 2018

@wycats Finished up a test for broken component behavior in didRecieveAttrs here, and found another test that has been skipped that demonstrates the issues we'll have with constructors here. I'm going to start adding tests for proto/observer behavior next.

@wycats
Copy link
Member Author

wycats commented Jan 18, 2018

@pzuraq this is great work. Keep em coming!

@amiel
Copy link
Contributor

amiel commented Jan 18, 2018

App 1

Total services: 21

is an object proxy: 4
injects another service: 6
alias: 0
readonly: 6
non-volatile computed properties: 6
observers: 0
concatenated or merged properties: 0
actions: 0
ember-concurrency tasks: 2

App 2 (older)

Total services: 16

is an object proxy: 1
injects another service: 11
alias: 5
readonly: 12
non-volatile computed properties: 9
observers: 0
concatenated or merged properties: 0
actions: 0

@backspace
Copy link

Hey, exciting work! For travis-web:

Total services: 23
is an object proxy: 0
injects another service: 14
alias: 3
readonly: 0
non-volatile computed properties: 12
observers: 1
concatenated or merged properties: 0
actions: 0

I wasn’t sure about “is an object proxy”; some services are extensions of others, but I think that isn’t what you mean?

I also wasn’t sure whether “non-volatile computed properties” should include alias, but I guessed not.

@wycats
Copy link
Member Author

wycats commented Jan 22, 2018

@backspace alias was listed separately, so no 😄

"is an object proxy" means ObjectProxy.extend()

@nathanielw
Copy link

Total services: 31
is an object proxy: 0
injects another service: 27
alias: 0
readonly: 0
non-volatile computed properties: 28
observers: 0
concatenated or merged properties: 0
actions: 0

EventedMixin: 5
ember-concurrency tasks: 4

@kevinansfield
Copy link
Contributor

Counts for Ghost:

Total services: 17
is an object proxy: 3 (using `_ProxyMixin`)
injects another service: 12
alias: 0
readonly: 1
non-volatile computed properties: 6
observers: 0
concatenated or merged properties: ?
actions: 1

EventedMixin: 4

@billybonks
Copy link
Contributor

TradeGecko total services and computed properties are going to grow by quite a few more.

Total services: 78
is an object proxy: 0
injects another service: 40
alias: 8
readonly: 6
non-volatile computed properties: 49
observers: 0
concatenated or merged properties: 0
actions: 0

@wycats
Copy link
Member Author

wycats commented Jan 25, 2018

This is great!

It looks like by far the dominant use-cases are:

  • service injection
  • computed properties
  • simple computed property macros like alias and readonly

That's really great news, because those features have a clearer path to direct migration to ES6 classes than some other features of the Ember object model (like concatenated properties).

Keep the surveys coming.

@knownasilya
Copy link
Contributor

What about actions on services. I think this feature might be unused mainly due to education and that most people don't know about {{action ... target=..}}. In my opinion this is an untapped feature, especially with immutable state on the service. Here's an example: https://ember-twiddle.com/228b63aeb8f8e6464960ba019d7ce720

I'd love for actions to work with functions and be bound to the parent object by default, that would prevent having to need an actions object.

@wycats
Copy link
Member Author

wycats commented Jan 25, 2018

@knownasilya My personal opinion is that, in modern Ember, actions are just functions, and that the point of the {{action}} helper is to bind the function for you. Therefore, we don't need to worry about old-style actions at all in new code. What do you think?

@rwjblue
Copy link
Member

rwjblue commented Jan 25, 2018

My personal opinion is that, in modern Ember, actions are just functions, and that the point of the {{action}} helper is to bind the function for you.

{{action definitely does not have the semantics that are inferred here. Interestingly, I have also taken this perspective and was "out voted" on actually fixing the semantics for this. Please review #14479 for some of the history there...

I'd suggest at this point, for simple binding purposes that folks should use the {{bind helper (provided by https://github.com/Serabe/ember-bind-helper) rather than {{action.

Therefore, we don't need to worry about old-style actions at all in new code. What do you think?

TBH, I don't think its this simple. We might be writing "new code" by authoring a replacement for an existing concept (consider refactoring to a service from a controller). When the "new code" we are talking about is template code: I totally agree! But to have to refactor any number of my templates simply to refactor to a service seems non-ideal.

@bendemboski
Copy link
Contributor

Total services: 13
is an object proxy: 0
injects another service: 10
alias: 2
readonly: 3
computed properties (not volatile): 8
observers: 0
concatenated or merged properties: 0
actions: 0

@iamdtang
Copy link

Total services: 30
is an object proxy: 0
injects another service: 18
alias: 2
readonly: 2
computed properties (not volatile): 30
observers: 1
concatenated or merged properties: 0
actions: 0

@karthiicksiva
Copy link
Contributor

Total services: 18
is an object proxy: 0
injects another service: 10
alias: 0
readonly: 0
computed properties (not volatile): 6
observers: 0
concatenated or merged properties: 0
actions: 2

@dan-ste
Copy link

dan-ste commented Jan 29, 2018

Total services: 31
is an object proxy: 0
injects another service: 25
alias: 2
readonly: 3
computed properties (not volatile): 2
observers: 0
concatenated or merged properties: 0
actions: 0

@SergeAstapov
Copy link
Contributor

Total services: 47
is an object proxy: 0
injects another service: 39
alias: 13
readonly: 6
non-volatile computed properties: 60
observers: 0
concatenated or merged properties: 0
actions: 0

@whatthewhat
Copy link
Contributor

Total services: 32
is an object proxy: 0
injects another service: 18
alias: 0
readonly: 0
non-volatile computed properties: 6
observers: 0
concatenated or merged properties: 0
actions: 0

@raido
Copy link
Contributor

raido commented Jan 31, 2018

Total services: 10
is an object proxy: 0
injects another service: 6
alias: 0
readonly: 1
computed properties (not volatile): 0
observers: 1
concatenated or merged properties: 0
actions: 0

@chriskrycho
Copy link
Contributor

Olo’s main Ember app (~20k LoC in the app dir for context/scale):

Total services: 22
is an object proxy: 0
injects another service: 10
alias: 1
readonly: 0
non-volatile computed properties: 21
observers: 0
concatenated or merged properties: 0
actions: 0

@robwebdev
Copy link

robwebdev commented May 17, 2018

Total services: 16
is an object proxy: 0
injects another service: 7
alias: 0
readonly: 0
non-volatile computed properties: 1
observers: 0
concatenated or merged properties: 0
actions: 0

EventedMixin: 4

@sevab
Copy link

sevab commented May 17, 2018

I wrote a simple ruby script as part of Ember London's Meetup Quest Night to test all services against the listed features: https://gist.github.com/sevab/497ead350f25e82c5034686c6b163192

Matches Ghost's results. Feel free to try against your apps by dropping and running the file at the root of the project.

@vitch
Copy link
Contributor

vitch commented May 17, 2018

I was also at the Ember London's Meetup Quest Night and put together a script to generate these counts... Mine is javascript and you can paste it into the web developer console next to any running ember app and it should find the ember app and introspect it and output the relevant info.

https://gist.github.com/vitch/591d022359bd392535c15c715a59e81a

Here's the output from one of our apps:

Total services: 72
is an object proxy: 0
injects another service: 58 properties in 46 services
computed properties: 149 properties in 52 services
alias: 5 properties in 4 services
readonly: 4 properties in 2 services
involatile computed properties: 148 properties in 52 services
volatile computed properties: 1 properties in 1 services
observers: 1 properties in 1 services
concatenated or merged properties: 0 (?)
actions: 0

The script is hacked together and may not work on all ember apps or all versions of ember but hopefully it's a useful start. It would be pretty easy to extend it to output additional information if desired...

@s-chiriac
Copy link

s-chiriac commented May 18, 2018

Total services: 76
is an object proxy: 0
injects another service: 215 properties in 59 services
computed properties: 149 properties in 53 services
alias: 132 properties in 46 services
readonly: 5 properties in 4 services
involatile computed properties: 129 properties in 52 services
volatile computed properties: 20 properties in 18 services
observers: 7 properties in 6 services
concatenated or merged properties: 0 (?)
actions: 0

Another of our apps:

Total services: 20
is an object proxy: 0
injects another service: 0 properties in 0 services
computed properties: 0 properties in 0 services
alias: 0 properties in 0 services
readonly: 0 properties in 0 services
involatile computed properties: 0 properties in 0 services
volatile computed properties: 0 properties in 0 services
observers: 0 properties in 0 services
concatenated or merged properties: 0 (?)
actions: 0

@vitch
Copy link
Contributor

vitch commented May 18, 2018

I should have mentioned before too many people use the script... I wasn't clear on what "concatenated or merged properties" was meant to represent so that will always output "0 (?)" for now! If someone can tell me what it's meant to represent I can update the script.

I can also try to add information about the Evented Mixin if that's useful. And I was thinking it might be nice to distinguish services defined in the app from those defined in addons?

@pzuraq
Copy link
Contributor

pzuraq commented May 18, 2018

Concatenated and merged properties are a feature of the object model that’s mainly used for components, controllers, and routes. Examples are actions and classNames, when you define these on a subclass they merge with the superclass’s value instead of overwriting it.

It’s actually possible (but highly unrecommended) to define your own properties that do this. We didn’t expect anyone to be using this feature, but wanted to make sure.

@vitch
Copy link
Contributor

vitch commented May 28, 2018

I wasn't sure how they manifested themselves on the services but there seems to be concatenatedProperties and mergedProperties properties on all of the services which are an empty array in all cases on my test app.

I updated my script to output information about these as well and here's the updated list from one of our apps:

Total services: 72
is an object proxy: 0
injects another service: 58 properties in 46 services
computed properties: 148 properties in 52 services
alias: 5 properties in 4 services
readonly: 4 properties in 2 services
involatile computed properties: 147 properties in 52 services
volatile computed properties: 1 properties in 1 services
observers: 1 properties in 1 services
concatenated properties: 0 properties in 0 services
merged properties: 0 properties in 0 services
actions: 0

@pixelhandler
Copy link
Contributor

@wycats Is there a Discord channel this work continues on?

We'll be coordinating this work on the community slack. You can join the #st-es-class-services channel.

@pzuraq
Copy link
Contributor

pzuraq commented Sep 28, 2018

@pixelhandler most of the active work is happening in #st-native-classes now, and there is a new quest issue which has the most up-to-date information about the state of native classes, #16927

The goals have shifted a bit from just services without a base class to using native classes with the Ember Object model for the time being, with a longer-term outlook outlined in the Native Class Roadmap RFC

@pixelhandler
Copy link
Contributor

@pzuraq should this one be closed?

@pzuraq
Copy link
Contributor

pzuraq commented Oct 19, 2018

I think so, we should double check with @wycats but I think baseless native classes still need to be fleshed out and maybe RFCd before we can move forward with them

@locks
Copy link
Contributor

locks commented Mar 26, 2019

The cheese has been thoroughly moved and this issue can be considered stale.

@locks locks closed this as completed Mar 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests