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

Explicit Service Injection #502

Open
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
@NullVoxPopuli
Copy link
Contributor

commented Jun 15, 2019

rendered

NullVoxPopuli added some commits Jun 15, 2019

@webark

This comment has been minimized.

Copy link

commented Jun 15, 2019

@NullVoxPopuli Where does the class name come from? Is it just the path camel cased with the type tacked on? Would it be something you import? Is it the class name you define in the individual file that would get pulled out?

@flashios09

This comment has been minimized.

Copy link

commented Jun 15, 2019

hi @NullVoxPopuli ,
i like the idea 👏
this will add some imports but it will make the code more clear/readable
i wish if we can do the same with a model
so instead of this:

this.store.findAll('post', '...')

we have:

@service(StoreService) store;
// i prefer the pluralized form, `posts` not `post` for model name
@model(PostsModel) posts;
// ...

doSomething() {
    this.store.findAll(this.posts, '...');
    // or
    this.posts.somePostsModelMethod('...');
   // maybe
   this.posts.findAll('...');
   // a class cased form for model name `Posts` or `PostsModel`
   this.PostsModel.findAll('...');
}
@buschtoens

This comment has been minimized.

Copy link

commented Jun 15, 2019

A few tangents regarding TypeScript.

Currently I (we?) do this for TypeScript:

import Service, { inject as service } from '@ember/service';
import BarService from './bar';

class FooService extends Service {
  @service bar!: BarService;
}

Alternatively you can get around the extra import of BarService, if you provide the service name explicitly and use the old Ember Object Model:

import Service, { inject as service } from '@ember/service';

class FooService extends Service.extends({
  bar: service('bar')
}) {
}

This works because of the clever registry pattern. The reason you have to use the Ember Object Model is that decorators still can't yet change the type signature: microsoft/TypeScript#4881

Once they could, the following should type-check and infer the service class automatically:

import Service, { inject as service } from '@ember/service';

class FooService extends Service {
  @service bar;
}

For the time being, if you don't want to or can't use the Ember Object Model, but dislike explicitly importing the injtectee class, you could use the trick that we discussed in machty/ember-concurrency-decorators#50: Use a Babel transform to convert class property assignments to decorated properties.

import Service, { inject as service } from '@ember/service';

class FooService extends Service {
  bar = service('bar');
}

// gets transformed into

class FooService extends Service {
  @service('bar') bar;
}

I have a transform for it ready, but I need to battle-test and optimize it further:
babel-plugin-transform-class-property-assignment-to-decorator

@gossi

This comment has been minimized.

Copy link

commented Jun 15, 2019

@buschtoens

Alternatively you can get around the extra import of BarService

if I understand this RFC correctly, it is about explicitely importing the respective class and exchange the string lookup for the class definition.

Second is: As much as we love typescript, this RFC must work for JS and as such the registry pattern from e-c-ts isn't available in pure js land. So with this RFC TS code will look like this:

import Service form '@ember/serivce';
import NotificationsService from 'my-project/services/notifications';

class Foo extends Service {
  @service(NotificationsService) notifications: NotificationsService;
}

at least until decorators can mutate the type definition for properties (then we can get rid of the doubled written class name).

@buschtoens

This comment has been minimized.

Copy link

commented Jun 15, 2019

@gossi This is exactly why I raised these points. While Ember is committed to not pushing TypeScript onto anybody and offering first-class support for JS, we are equally committed to offering first-class support for TypeScript as well.

I find it important that, as long as decorators cannot change types in TS or using a Babel transform is not a community-accepted and agreed upon opinion, we do not deprecate string lookups, as this could worsen ergonomics for some TypeScript users.


Edit: I actually believe we should never deprecate string lookups, as this would break the automatic inferral, when decorators can change signatures, i.e.:

import Service, { inject as service } from '@ember/service';

class FooService extends Service {
  @service bar;
}
@buschtoens

This comment has been minimized.

Copy link

commented Jun 15, 2019

One further observation: The Ember Resolver / Container system is a bit of magic in the background. You don't always know where the backing injectee class is actually located or it might not be accessible to the code you are authoring.

There's also nothing preventing you from registering the same class with multiple names or even generating the names at runtime.

Admittedly these are uncommon edge cases.

@lougreenwood

This comment has been minimized.

Copy link

commented Jun 15, 2019

Also, by requiring explicit class name use, we're coupling the class to a specific service, but this shouldn't be a concern of the class:

Dependency injection is one form of the broader technique of inversion of control. The client delegates the responsibility of providing its dependencies to external code (the injector). The client is not allowed to call the injector code;[2] it is the injecting code that constructs the services and calls the client to inject them. This means the client code does not need to know about the injecting code, how to construct the services or even which actual services it is using; the client only needs to know about the intrinsic interfaces of the services because these define how the client may use the services. This separates the responsibilities of use and construction.
https://en.wikipedia.org/wiki/Dependency_injection

I mean, if we're going to import the ClassName, why not instantiate the service singleton and skip the service() container lookup all together.... But we all know that's a bad idea as it removes IoC.

So whilst I like the look of this as purely a nicer way to write TS classes in Ember right now - it's not really DI any more since the explicit class name is required (no more IoC).

So it seems that if the goal is to improve typing in TS, proper TS support for decorators will give us that.

For newbies that get confused by strings, maybe we need to better teach the newbies about DI and why a string name is about as explicit & accurate a DI lookup name should be to maintain proper separation.

@buschtoens

This comment has been minimized.

Copy link

commented Jun 15, 2019

@lougreenwood I totally agree with the point you're making regarding isolation of concerns.

However, I think @NullVoxPopuli was not aiming at better TypeScript support, but instead better "cmd+clickability" support for generic JS IDEs / users that don't use TypeScript.

Please correct me, if I am wrong.

@lougreenwood

This comment has been minimized.

Copy link

commented Jun 15, 2019

yeah - you're right, I was getting ahead of myself - sorry :D

But IMO, we also shouldn't start hacking apart established patterns for nicer IDE support - our solution should "work with the patterns" as well as "using the platform".

So if I understand JS & IDEs correctly... Since JS is not typed, and by it's nature DI de-couples - then there's no "platform" mechanism which is true to DI other than typing (and decorators changing function signature) which allows nice IDE integration?

@NullVoxPopuli

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2019

@webark the class would be an import of the service itself :)

import Component from '@glimmer/components';
import { inject as service } from '@ember/services';
import MyService from 'appmame/services/my-service';

export default class extends Component {
  @service(MyService) myService;
}

:)

@NullVoxPopuli

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2019

@buschtoens the registry pattern could be used with classes as keys, couldn't it?

@NullVoxPopuli

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2019

then there's no "platform" mechanism which is true to DI

If only we had interfaces we could use instead :)

@NullVoxPopuli NullVoxPopuli changed the title Explicit Dependency Injection Explicit Service Injection Jun 15, 2019

@NullVoxPopuli

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2019

Renamed RFC due to technicality in DI definition

@buschtoens

This comment has been minimized.

Copy link

commented Jun 15, 2019

the registry pattern could be used with classes as keys, couldn't it?

@NullVoxPopuli Yes, of course it could. But that's not the point I was trying to make. 🙂

Switching to a class-based registry / decorator and deprecating the string-based one, means that the future TypeScript ergonomics will be much worse, because you can't any more use the property name to infer the injection, as show in #502 (comment).

@NullVoxPopuli

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2019

Gotchya, so string lookup stays :)

@webark

This comment has been minimized.

Copy link

commented Jun 15, 2019

the class would be an import of the service itself

Thanks for clearing that up!

So there’s a pattern where you extend a dependencies service, to either overwrite or extend the dependent service.

When this is done, all of your existing references are updated.

With direct imports, would you always import a dependents service from the addons merged “app” space, even if it doesn’t exist? Or would you need to go and update all of your imports where you had imported them from the depended addon?

@NullVoxPopuli

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

would you always import a dependents service from the addons merged “app” space, even if it doesn’t exist? Or would you need to go and update all of your imports where you had imported them from the depended addon?

it should resolve to the same thing in both scenarios, afaik

@buschtoens

This comment has been minimized.

Copy link

commented Jun 16, 2019

With direct imports, would you always import a dependents service from the addons merged “app” space, even if it doesn’t exist? Or would you need to go and update all of your imports where you had imported them from the depended addon?

it should resolve to the same thing in both scenarios, afaik

An addon that has a service re-export in its app tree would make the service available in the host app, via the resolver, however the file would not be physically "there", meaning that "cmd+click" would fail, while the import would work at runtime.


I don't want to explode the scope here, but this segways into another interesting problem: What is the "official" way (pre and post this RFC) to override / clobber an addon's service (from another addon possibly)?

One way is using ember-addon.after in the package.json of the overriding addon to ensure that its app tree clobbers the app tree of the overridden addon. You could do the same from the host app as well.

One major hazard of this approach is babel/ember-cli-babel#240: If you use a file extension other than js, the clobbering might fail non-deterministically.

@NullVoxPopuli

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

I don't want to explode the scope here, but this segways into another interesting problem: What is the "official" way (pre and post this RFC) to override / clobber an addon's service (from another addon possibly)?

maybe this RFC should be held off until embroider ships ;)
(though, I need to see if embroider would actually fix that problem)

but, I don't think we should continue to have clobbering of services. It's hard to debug.
if I import a service form an addon, I want that to be the actual location so I can ctrl/cmd click it and see what it is.

@webark

This comment has been minimized.

Copy link

commented Jun 16, 2019

An addon that has a service re-export in its app tree would make the service available in the host app, via the resolver, however the file would not be physically "there", meaning that "cmd+click" would fail, while the import would work at runtime.

this just goes against the main motivation of “enable "go to definition" support from service definitions so developers can more easily discover the where and how their service is defined.”

For using services in addons, it seems just as magical also, and more confusing around “is this a singleton” especially if you could import the same service from the invisible “app” space, and where it lives in the addon’s “addon” space in the same app.

@mehulkar

This comment has been minimized.

Copy link

commented Jun 16, 2019

This RFC should also probably provide some text about how to register/inject services in component rendering tests.

@lifeart

This comment has been minimized.

Copy link

commented Jun 19, 2019

ctrl+click work in unstable language server )

@NullVoxPopuli

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

ctrl+click work in unstable language server )

no need if TSServer handles it :)

(but we'd need to make updates for appInstance integration, yeah?)

@lupestro

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

I'm a little concerned about supporting the true DI case where early during application startup we choose between independent implementations of the same (duck-typed) interface to register based on deployment-time data. You set it up in the registry once and no longer care which class was supplied. The examples all seem to assume there is a single "true" implementation and all the DI is for test stubs.

@NullVoxPopuli

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

@lupestro, it'd work if the same class key was used.

The main difference is that a class is used instead of a string. There technically doesn't have to be any correlation. But I'm recommending that at the very least the concrete class is a subclass if not the OG implementation

@mehulkar

This comment has been minimized.

Copy link

commented Jun 20, 2019

The main problem being solved here is traceability. “Why does inject with this string mean that this Singleton is available?”

An alternative solution (and less disruptive I think), would be to instead remove the inject import, and require that the service is looked up and set as a property in the constructor. As a parallel to that, the registration of the Singleton into the container should also be explicit in a app boot hook.

This would solve the traceability problem, reduce the surface area of the framework, and also retain “true DI” as others have pointed out.

If nothing else, maybe this can be added to the Alternatives section of the RFC?

@NullVoxPopuli

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

An alternative solution (and less disruptive I think), would be to instead remove the inject import, and require that the service is looked up and set as a property in the constructor. As a parallel to that, the registration of the Singleton into the container should also be explicit in a app boot hook.

Can you provide some examples?

@mehulkar

This comment has been minimized.

Copy link

commented Jun 20, 2019

I'm imagining something like in the component (sorry for not Octan-ifying the example code, I'm in a hurry)

export default Component.extend({
  init() {
    this._super(...arguments);
    this.set('foo', getOwner(this).lookup('service:foo'));
  }
});

and this in app/app.js in the default blueprint

Application.extend({
  onBoot(appInstance) {
    const serviceClasses = discoverServices();
    serviceClasses.forEach(s => appInstance.register(`service:${service.name}`);

    // similar things for components and everything else that can be looked up from the container
  }
});

Two things:

  • I don't know if the semantics of setting the service in init/constructor are the same as the inject import
  • The onBoot hook is completely made up and any resemblance to existing mechanism is purely coincidental. HOWEVER, I think "explicit" registration of things in the container as part of the default blueprint from ember new would solve many of the "magic" problems (and would also unlock more experimentation for many things!)
@pzuraq

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

@mehulkar I'm not sure that solution would end up being much more traceable, and it would incur a lot of extra overhead. You would essentially always have to go to the app/app.js file to find the definition of a service, since it could be coming from anywhere. The language server wouldn't be able to pick up on it without a lot of custom rules, which would probably be brittle.

I think the value of explicit, import based DI is it makes it very clear what the system is doing. You are injecting an instance of this class - that can be inferred pretty easily, even by someone who has no knowledge of an Ember app. They can also find that class pretty easily, given the definition. They don't have to know any implicit file lookup rules, or the way that a particular application has configured its service registrations (which, if it's done manually, will likely become bespoke per app).

Re: @lupestro's concerns, I think that while this is relatively uncommon (as in, the majority of services do only need one implementation), it's definitely a problem we need to consider. I really like the idea that if you want to inject a service class, then the actual implementation must either be the class, or a subclass. This would allow us to implement an abstract class for the definition, with concrete implementations that extend from it:

// Typescript
abstract class CookieService {
  abstract getValue(key: string): string {}

  abstract setValue(key: string, value: string): void {}
} 

// Component definition
class MyComponent extends Component {
  @service(CookieService) cookie;
}

// Impl
class FastbootCookieService extends CookieService {
  // ...
}

class BrowserCookieService extends CookieService {
  // ...
}

// Initializer
export function initialize(appInstance) {
  if (isFastboot()) {
    appInstance.register(CookieService, FastbootCookieService);
  } else {
    appInstance.register(CookieService, BrowserCookieService);
  }
}
@mehulkar

This comment has been minimized.

Copy link

commented Jun 20, 2019

I think the value of explicit, import based DI is it makes it very clear what the system is doing.

Just to be clear, I would prefer this also 😄, as long as there continue to be a way to inject a replacement (in tests or at build time as @lupestro brought up).

The language server wouldn't be able to pick up on it without a lot of custom rules, which would probably be brittle.

Yep, same situation as now, I presume.

I'm not sure that solution would end up being much more traceable,

Maybe not today, since app.js is a largely ignored file. But I think app/app.js could be a much more useful entry point for Ember projects in general. Preparing the container and "discovering" core classes like components and services would be a very useful part of the default blueprint and give devs a clearer understanding of Ember's mental model. But I agree that that's a much larger discussion and could have other side effects that can't be explored as part of this RFC.

and it would incur a lot of extra overhead.

For learning or for performance? From a learning standpoint, I don't think it's unreasonable to:

  1. have a mental model of a container
  2. know that things get put into the container at some point
  3. know that other parts of your app look these things up in that container

We already understand 1 and 3. It's just 2 that's invisible and is the part that isn't traceable.

If the blueprint continues register the default export from app/services/foo.js as service:foo, the convention promise is met, and the dev gets more freedom and better understanding of what's happening (which is what this RFC is all about, IMO).

You still never have to look at app.js though. Injections can work the exact same way.

Anyway, I'm all for injecting imported classes! This alternative makes a lot of sense to me and also paves the way for other things to become more explicit (like initializer and instance initializer functions). Requiring a base class for the weakmap also makes sense. Services are already required to be extended from Ember.Service (pre-native classes, anyway), so this shouldn't preclude any existing use cases that I can think of.

Thanks for hearing me out!

@mehulkar

This comment has been minimized.

Copy link

commented Jun 20, 2019

Actually, one other point about the common base class as a weakmap key...

@pzuraq your example of having an abstract base class is great, but I worry that people will extend the real class to create the stub, rather than first extract an abstract class.

For example, if you have a service with 100 methods in it and you want to write an isolated test that will make use of one of those methods. If you extend from the real service and stub out that one method, you'll still end up with 99 other methods that will eventually leak and break your test. This would be either be a very bad pattern to set or be asking for a lot of overhead for devs to create an abstract class before writing a test.

@pzuraq

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

and it would incur a lot of extra overhead.

For learning or for performance?

I actually meant for:

  1. Tooling
  2. Day-to-day app development

If we were to force manual registration in app/app.js, like you're suggesting, even if the blueprints were to manually add the registrations, the fact is that in time and in very large applications, they will get out of sync with the actual folder structure, import paths, etc. I've had to do this for statically typed languages, and even when you have the type system helping you, it becomes a mess pretty quickly.

It essentially will mean that for every single service you see in an app, if you want to find it you will have to:

  1. Open up app/app.js
  2. Look for the service that is registered
  3. Look for the import path of that service
  4. Open up the actual file

Sometimes you'll be able to skip some of these steps based on conventions, yes, but we won't be able to every time, nor will you be able to write general purpose tooling that could take you through it with a click, since any custom registration could occur at any point.

FWIW, this is actually closer to where we used to be, before we had the service() injector/decorator. You would have to manually register values in the container in app instance initializers (which is likely where this type of wiring would go) and you would have to manually look up registrations. I think service() was a valuable iteration on that period, it really cleaned up Ember and made it much easier to understand. It did this, IMO, because it didn't violate the "as-above-so-below" rule as I call it - you should never have to traverse to the other side of an application/repository/library/ecosystem to understand what is going on locally.

@pzuraq

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

but I worry that people will extend the real class to create the stub, rather than first extract an abstract class.

For example, if you have a service with 100 methods in it and you want to write an isolated test that will make use of one of those methods. If you extend from the real service and stub out that one method, you'll still end up with 99 other methods that will eventually leak and break your test. This would be either be a very bad pattern to set or be asking for a lot of overhead for devs to create an abstract class before writing a test.

Honestly I think if you have a service that has 100 public API methods, you probably have too big of a service 😄 There are also plenty of cases where you don't want to override all of a services implementation methods, so I think it's a totally valid pattern to extend a service without creating an abstract base class.

The point on stubbing is to create a service that behaves more or less the same externally, but doesn't have certain side-effects. It's usually better to keep most of the service's logic the same, and isolate the parts that need to be changed for tests. This way, you tightly close over the domain in a way that is transparent to the test suite. A great example is Pretender - it completely hides the fact that it's mocking XHR requests.

@kimroen

This comment has been minimized.

Copy link

commented Jun 20, 2019

Having worked a bit with Angular in the past few months, looking at how dependency injection and services works there was pretty interesting:

import MyService from '../services/my-service';

class MyComponent extends Component {
  constructor(
    private myService: MyService
  ) {}
}

this.myService now works inside this component.

As far as I can tell this though, this is entirely based on it being TS so it's not necessarily something we could use directly, but maybe there's something to draw inspiration from?

@NullVoxPopuli

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

@kimroen, that's how it works in asp.net as well, and was def part of the inspiration that went into this :)

NullVoxPopuli added some commits Jun 20, 2019

@chriskrycho

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

Having worked a bit with Angular in the past few months, looking at how dependency injection and services works there was pretty interesting:

import MyService from '../services/my-service';

class MyComponent extends Component {
  constructor(
    private myService: MyService
  ) {}
}

this.myService now works inside this component.

As far as I can tell this though, this is entirely based on it being TS so it's not necessarily something we could use directly, but maybe there's something to draw inspiration from?

That’s correct; they only make their DI system work this way by emitting runtime metadata from TS (including decorators in particular). That’s obviously a non-starter for Ember.

@pzuraq

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

Something we have discussed is in the future, if decorators land and eventually, parameter decorators are proposed and land (could take a while), we could do something like:

import MyService from '../services/my-service';
import { inject as service } from '@ember/service';

class MyComponent extends Component {
  constructor(
    @service(MyService) myService
  ) {}
}

Like I said, this probably won't happen for years to come, so I definitely wouldn't want to include this type of thing in this RFC, but I'm mentioning it because it's definitely part of the (far future) design space 😄

@pzuraq

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

Over the weekend I hacked out an addon that implements this RFC using Ember's public APIs, ember-totally-not-module-based-services (the name is meant to be humorous, see the README 😛). I figured it'd be a good idea for us to have some reference for how this RFC would actually work, and a place to experiment with the API to see if it's ergonomic or not, and what it's shortcomings might be. I also made an @abstract decorator for defining abstract classes, which could be handy for following the patterns that have been described in this RFC.

After more thought and playing around with the API, I think the biggest concern I have so far is that we don't have a simple, declarative way to override a service at the application level. Today, if you need to override a service foo to add some functionality, you can create the services/foo.js file and you're done with it. For cases with multiple implementations, like fastboot, this was never so simple, since you would have to have some branching logic somewhere such as an instance initializer, but it does seem like adding the step of creating an initializer just to override a service would be a bit of unnecessary boilerplate.

I've been toying around with the idea of a @register or @override decorator, something that we could use the declaratively tell the DI system that this class is meant to be registered at a particular key:

@override({ key: Store })
class MyCustomStore extends Store {}

It could even have conditionals:

@override({ key: CookieService, if: !isFastboot() })
class BrowserCookieService extends CookieService {}

The duplication of the key in these cases wouldn't be ideal, since you're both extending and registering it, but the declarative nature of a class decorator would be nice because it actually is statically analyzable (unlike initializers), and would also get us away from relying implicitly on the ordering of the build system. I do think it gets tricky if you try to allow addons to use a decorator like this as well - if both the app and an addon (or two addons) attempt to override the same service, we have an ordering problem once again. This could also come later on, or in parallel to this RFC - it would work just as well with string based service keys, and would have the same benefits of getting us off using the build system for resolution if we can nail down the semantics.

@chriskrycho

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

FWIW, on this point:

Today, if you need to override a service foo to add some functionality, you can create the services/foo.js file and you're done with it.

I very much understand why people reach for this, but every time I've seen it, it's an anti-pattern—the amped up version of the failure which "prefer composition over inheritance" is meant to address. It may be a thing we want to support as an escape hatch, because sometimes addon's services are badly designed, but even there, simply injecting it into one's own service and providing a wrapper API around it seems to me to be a much better pattern than the override-and-it-happens-to-have-the-sam-ename pattens we currently use.

The trick, of course, is for the scenario where you want it to be resolved back in the original addon's code or similar, but that also seems like a very, very smelly smell to me.

I don't have more fully-formed thoughts than that, but I figured I'd raise it as a point against the importance of that kind of "overridability".

Semi-related: perhaps the design of a new system here should not assume it, so as to be backwards compatible, but it seems to me that accounting for a future where addons' exports are no longer merged into the consuming app's namespace may illuminate a better design here. If you don't have namespace merging, you also don't have the ability to override in the same way, and new patterns will need to emerge to account for the kind of escape-hatch work referenced here. Again, we may not want to defer this work until that time, but leaving it as an intentional hole in the design—acknowledged and unanswered, designed to be filled in later when we have more of an idea how that system will play out—may be a reasonable option.

@simonihmig

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2019

I have some concerns with this, mostly along the lines of what @lougreenwood and @lupestro said before.

While the RFC is mostly concerned about basically what the key for a lookup operation is (a string or a class), this small-ish change can only properly be assessed when we are clear about what the scope of our DI system is, and where it is eventually heading to:

1. Singleton container with test mocking
In this scenario the assumption would be to have one single implementation for a given service, and you already now where it is (in /app/services/name.js), so you know where to import it from. But as you don't want to create separate instances, our "DI" system's main purpose is to manage singleton instances. And additionally have some means to switch implementations mainly during tests.

This is probably how we use Ember's DI in most cases, but effectively that is a very limited form of DI. And given the dynamic nature of JS, we could mock services in tests even without any dynamic registering/lookup.

2. True DI
In "True DI"(TM) the system would not make any assumptions of where a dependency comes from, and how it is implemented. The explicit goal of Inversion of Control (IoC) is to foster decoupling. So having to import a concrete implementation of a service just to declare a dependency on any implementation that satisfies the needed interface is counter-productive, as it increases coupling. Rather neither the dependency nor the consumer should be aware of each other (other than through the interface). The consumer ideally should just declare a dependency on some interface, which the DI needs to inject an implementation for.

The obvious problem we have here is that our platform does not know the concept of an interface (even TS has that only at build-time). So IMHO our string-based lookup actually comes closer to that than any injection based on a concrete implementing class. Even an abstract class seems wrong to me, as to my understanding that is still concerned on an (incomplete, sharable in a class hierarchy) implementation rather than defining an interface.

Let's look at an example:

We want to have a central logger service, but with different implementations based on context:

  • ConsoleLogger for development
  • SentryLogger for production
  • WinstonFileLogger in FastBoot

Now we have an auth service that needs to log things. What logger implementation should that import for the injection declaration? This get's even worse when an addon's service needs a logger injection, how would it be able to decide which class to import?

Having an abstract class for that was a suggestion, but as said before, I don't see an abstract class as a valid replacement for some other way to declare a dependency on an interface.

And I also see some practical problems with such an (abstract-class based) approach. Ideally services wouldn't need to extend from Ember.Service or any other common (abstract) class anymore. Instead two possible different implementations could come directly from two different npm packages, not extending Ember.Service or having any other relation to Ember at all, as long as they satisfy the same interface.

TL;DR

While our current DI is commonly used in a way as described under 1., maybe that is because it is IMO quite limited still. At least I would love to see it evolve to be more flexible in a "true DI" sense. And in that regard the proposed changes here seem counter-productive, as they increase coupling while IoC is supposed to decrease it. And as such it would make teaching the benefits of IoC and loose coupling more difficult.

@NullVoxPopuli

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

At least I would love to see it evolve to be more flexible in a "true DI" sense. And in that regard the proposed changes here seem counter-productive, as they increase coupling while IoC is supposed to decrease it. And as such it would make teaching the benefits of IoC and loose coupling more difficult.

Kinda throwing this out there, but we could use decorators to fake a runtime interface -- as long as that object that declared the interface was only defined once, I think the "references" / "usages" aspect of today's tooling would still work.

However, this potentially adds runtime overhead, and makes the overall D.I. system more complicated both in implementation and usage. :-\

@chriskrycho

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

Much of this discussion has me wondering if it's not worth taking a step back and reconsidering the shape of this entirely. It's worth remembering that dependency injection and particularly DI containers are a specific way of implementing the much more general pattern of inversion of control. However, a commitment to this specific kind of DI container is not a hard necessity.

Functional languages with currying and partial application as native language constructs (F♯, Elm, etc.) tend to solve this simply by passing arguments which must conform to a given interface. And there are similar patterns available even in more traditional "OO" languages where the arguments are passed to the constructor.

That's not to say that we should switch to an FP style, per se—that would be a fairly radical shift in the programming model, and while I'd like it personally, it may or may not be the best migration path here! My point is simpler: if we're evaluating what the future of our inversion-of-control pattern should look like at this level, perhaps it's worth taking a more fundamental look at the options on the table for idiomatic JavaScript and thinking about how we might make a great developer experience there. A good migration path is not the same as an API that is effectively just a superset of today's API—that's just one possible path (if often a good one)!

@pzuraq

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

I had a long conversation with @runspired about this RFC and the future of Ember Data last week, and his concerns mirrored @simonihmig's concerns. We also came to the conclusion that what this RFC is after, ultimately, is to formalize services around interfaces. This is also how most major DI frameworks work in typed languages, so it makes sense that everyone kind of wants to be able to do this. Since we don't have interfaces in JS, classes are the next best thing, and abstract classes are the closest real pattern that we could use to emulate interfaces.

Another important point is that Ember Data is internally going to be moving a direction where True DI (@simonihmig's use case 2) is much more common, even required. Data will define interfaces rather than concrete services in the future, and various other addons will implement those interfaces in their own services. The ability to swap out implementations like this for a particular service is very valuable for an ecosystem, since it means that other addons and libraries can rely on the existence of a particular service for a particular key, declaratively.

The biggest concern we ended up having with using a class as the key is, even if it were an abstract class, tooling would link users to that class even if a different class were the selected implementation, and that could be very confusing. The Ember language server might be able to solve this with some clever redirects, but we also have to consider environments that won't be able to run the language server, such as Github (since it recently implemented go-to-definition).

We did land on a couple of deficiencies of the current system though, which explicit service injections would solve:

  1. The current system is intimately tied to the build process. If a service is provided by an addon, there's no easy way to determine where it is coming from. If multiple services are provided for the same name, there's no easy way to determine which one will "win". You have to know intimate details of the build to figure these problems out, and so does the language server if we want to have tooling for it.

  2. The most common and recommended way for addons to provide private, internal services is through this merging and build logic. It's not uncommon for addons to need a private service, and it's not uncommon for another addon to use this to override a those "private" services and extend them.

Both of these problems could be addressed, even if we stuck with string based keys. Our strawman solution was to:

  1. Require that all global services be re-exported in the main consuming application's /app/services folder (or the equivalent in any future file layout). This would give users a single location to look for all of their app's services, along with a place to select which implementation they want for a particular piece (e.g. using a GraphQL implementation of a particular Ember Data service interface).

  2. Have a method for addons to declare and use local, private services. This could be the existing syntax described in my previous post, or it could be a new syntax, but the main point is that these would not be leaked into the global namespace.

If we made these changes, then I think the main difference between strings and classes as keys would be DX around and support for go-to-definition. Strings would be supported via the language server, and simple to understand, but would not work in any environment by default (no Github support). Classes would work in most places, but could be confusing in cases where they're used to emulate interfaces. I still like the idea of being able to inject classes directly in the common case, but I can also see how it could add more complexity (or percieved complexity). I think ultimately, we should try to address these two concerns separately in different RFCs.

One last thought here: JavaScript doesn't have interfaces, but TypeScript does. It may be possible for ember-cli-typescript to implement a babel transform that takes something like:

@service foo: MyServiceInterface;

And transforms it into:

@service('my-service') foo;

Making the ability to "inject interfaces" a progressive enhancement of TS that doesn't impact JS APIs directly. How we would associate a string name with an interface would be tricky, but maybe this is a way we could get the best of both worlds.

@runspired

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

@simonihmig and @pzuraq collectively have done a very good job summarizing my positions.

I fully agree with @pzuraq that there are things we should do to tighten up the existing expectations such that knowing where a service in use was declared is quickly discoverable, and that doing so is separate from changing the nature of the DI system (which this RFC would do largely by accident) or the syntax of using the DI system.

One thing I'd note on the value of strings-v-classes as keys that I think got glossed over is that were we to cleanup the expectations around where services can originate from then when you don't have a language server it is still quick to find the source (look in the services directory for the service of that name, if it's a re-export you can go-to-source from there). E.g. if we need a language server for either pattern (which we would) I feel strings come with the additional benefit of conveying the location to go look for the concrete implementation when that language server is not present.

@NullVoxPopuli

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

I mean, really all I want is control+click. With strings, we need to make up our own tooling, which sounds like more work. :-\

@runspired

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

I had this discussion with @NullVoxPopuli out of band, but wish to summarize it here.

I mean, really all I want is control+click. With strings, we need to make up our own tooling, which sounds like more work. :-\

Moving to using classes as keys would not prevent us from using our own tooling nor save us any work. In either case we end up needing a language-server to ensure that control+click works.

For strings, we have to map strings to a class.
For classes, we have to map classes to other classes.

This remapping from class A to class B is likely to cause users a large amount of confusion. (Just imagine a new user being told that to "use service B" you must "import service A").

Why would this be?

Let's run through an example of some pitfalls with classes as keys.

Imagine the addon GoodCommunicator provides the service called central-hub, and in several of it's own other services or components it consumes that service.

export default class CentralHubService {
  dispatch() {}
  subscribe() {}
}

Let's say that GoodCommunicator is such a great addon that many other addons also try to coordinate some behaviors via the central-hub.

import CentralHubService from 'good-communicator/services/central-hub';

export default MyComponent extends Component {
  @service(CentralHubService) centralHub;
}

Still another addonGoodCommunicatorPlus realizes that the central-hub could use an upgraded protocol for dispatch, so it imports and extends the original class.

import CentralHubService from 'good-communicator/services/central-hub';

export default AdvancedCentralHubService extends CentralHubService {
   dispatch() {
      if (...) {
        // ....
      } else {
        return super.dispatch(...arguments);
      }
   }
}

And it (or another addon) uses this version of the service class:

import AdvancedCentralHubService from 'good-communicator-plus/services/central-hub';

export default MyComponent extends Component {
  @service(AdvancedCentralHubService) centralHub;
}

Now finally, a consuming application consumes GoodCommunicatorPlus as an addon and it decides to use the service, so it adds it to its services, adding in some config.

import CentralHubService from 'good-communicator-plus/services/central-hub';
import config from './hub-config';

export default ConfiguredCentralHubService extends CentralHubService {
   config = config;
}

The situation we are now in is that we have 3 distinct classes that all are serving as keys to what is intended to be the same service.

If the consuming application does nothing, then we would instantiate three separate services, two of which would be missing the desired configuration.

If however the consuming application wanted to ensure that we continued to only have a single service, it would need to remap the keys. Something like:

import CentralHubService from 'good-communicator/services/central-hub';
import AdvancedCentralHubService from 'good-communicator-plus/services/central-hub';
import ConfiguredHubService from '../services/central-hub';

export function initialize(application) {
  application.unregister(CentralHubService);
  application.unregister(AdvancedCentralHubService);
  application.register(CentralHubService, ConfiguredHubService);
  application.register(AdvancedCentralHubService, ConfiguredHubService);
};

export default {
  initialize
};

With this remapping complete, we would now be back to a single "central-hub" service.

But there remains a problem: the class we've used as a key in so many places is not the class that we've instantiated. Any "go-to-definition" features we take advantage of out of the box are going to send us to the wrong class for 2 of the 3 potential keys in this example. And remember, there's nothing preventing an application from using all 3 key variants (the two from addons and it's own).

In parting, a final concern:

This sort of keying by class is also problematic if someone desires multiple instances of a given service. By using the class as the key, we lose the ability to re-use the same class multiple times if desired.

@pzuraq

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

@runspired I think your examples are exaggerating the issue a bit, as they don't use the API the way it is described by the RFC. If we were to move forward with this RFC, in order for any addon to override a service, it would have to manually reregister that service, so the addon that provides the AdvancedCentralHubService would require users to do something like this:

import CentralHubService from 'good-communicator/services/central-hub';
import AdvancedCentralHubService from 'good-communicator-plus/services/central-hub';

export function initialize(application) {
  application.register(CentralHubService, AdvancedCentralHubService);
};

export default {
  initialize
};

And the addon itself would inject the CentralHubService:

// good-communicator-plus/components/my-component.js
import CentralHubService from 'good-communicator/services/central-hub';

export default MyComponent extends Component {
  @service(CentralHubService) centralHub;
}

I'm not particularly worried about this pattern being difficult or smelly, because IMO it should be. Attempting to override a service from another addon like this should require some manual steps, because it is generally an antipattern - the functionality should be contributed up stream, ideally, or contained in a separate service that the downstream addon has full control over.

Now, your second example is a bit more where my concerns come from as well, because it isn't uncommon to have a service that should be configured, or an interface for a service that should be provided by the consuming application, and Ember Data is moving this direction so it'll become even more common. In these cases, it's unavoidable to have a different implementation, and this alternate implementation would need to be registered, and then tooling would have to know about that registration so it could Control+Click to the proper definition. While we could absolutely do this, we no longer get it for "free", like this RFC was hoping for.

I think if we were providing interfaces, this would be a more logical leap. Users would go-to the definition, see it was an interface, and then know they had to find the proper implementation. I agree that taking them to a class, even an "abstract class", probably isn't enough of a hint that there could be a subclass being used, and could result in a lot of confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.