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

Pre-RFC: Deprecate implicit injections (owner.inject) #508

Open
rwjblue opened this issue Jun 28, 2019 · 11 comments

Comments

Projects
None yet
8 participants
@rwjblue
Copy link
Member

commented Jun 28, 2019

I'm defining "implicit injections" as the type of injection where nothing exists in the class body (or inherited from the parent class), but a property is magically present anyways. These types of implicit injects are nearly always done from initializers or instance-initializers (both of which should also die in a 🔥, but that is another issue). Examples of these implicit injections are using this.store from within an arbitrary Route / Service / Controller / Component without first specifying store: injectService().

Implicit injections have long been a source of confusion for new comers and old-timers alike.

Where does this.store come from?!

Since the advent of Ember.inject.service (from waaaaayyy back in Ember 1.9.0!) we have had a much better way to bring in external collaborating services like this. Explicit injections are massively easier to reason about since you can easily see that the class itself (or its ancestor) defines the injection.

As mentioned above the most used implicit injection is this.store (though there are others) which might indicate that this is "Ember Data's problem", but due to the nature of these implicit injections there is really no way that ember-data can actually deprecate usage of these implicit injections. If ember-data removes the initializer that sets up the implicit injections, it is clearly a breaking change. ember-data has no way to ensure that the store property emits a deprecation (but only if not "clobbered" by store: injectService()) which means that it can't even be removed in a major version bump due to Ember's unique SemVer commitment (to not remove any undeprecated public APIs in a major version bump).

We must provide a mechanism to deprecate these implicit injections wholesale. They are bad, they make our programs harder to understand, 🔥🔥🔥.


Concrete implementation suggestion:

  • Add an argument to owner.inject that allows the caller to specify that the injection is deprecated, which version it is deprecated until, a deprecation id, and a url for reading more about the deprecation. (For those of you following along, I've just described the options argument to @ember/debug's deprecate method 😉)
  • In debug builds, when this deprecation options argument is passed, Ember will automatically setup a getter for the deprecated injected property on the target object when the object is created.
  • Sometime later, issue a deprecation when the owner.inject is not provided that new extra "deprecations" object.

The goal of this pre-RFC is:

  1. To circulate the idea of pushing this forward
  2. Find an author that has the time to work with me on the final detailed design and put together the deprecation RFC
  3. Help with implementation of the RFC once landed
@efx

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

I am in favor of moving this forward. I and teammates ran into this problem recently from an initializer that indirectly prevented stubbing a service. The indirection, though self-inflicted, caused confusion that could be easily avoided.

  1. yes

Application initializers are run as your application boots, and provide the primary means to configure dependency injections in your application
Guides on Initializers

By establishing you access dependencies only through explicit injection we simplify the mental model and teaching it. It also paves the way to remove initializers.

@NullVoxPopuli

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

YAAAAS.

would part of this RFC also include updating the route/controller/etc blueprints to include things like

export default class MyExplicitRoute extends Route {
  @service store;
}

?

@rwjblue

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2019

would part of this RFC also include updating the route/controller/etc blueprints to include things like

TBH, I don't think so. It is very easy to add when you need it, and I don't particularly think that most files will use it (certainly not most controllers, but unsure RE: the breakdown in routes).

@rwjblue

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2019

It also paves the way to remove initializers.

Indeed, though I'd like to be very clear that I'm not specifically proposing that at this time. We might want to do it (I personally would like to), but I haven't thought through all of the possible reasons that folks use initializers/instance-initializers enough to be confident that we can address all use cases in other ways.

@efx

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

Indeed, though I'd like to be very clear that I'm not specifically proposing that at this time.

Completely agree. I think focusing the scope of the above change as sharply as we can will make it easier to design and implement successfully. Small steps.

@ef4

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

I’m in favor. This is a pretty strong requirement for making static services be a thing in embroider.

@jenweber

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

The "how we teach this" section should be pretty straightforward - add injecting the store into examples in the Guides and API docs. I think we may have already started doing this in some places.

Also there is one section here to be updated.

@chancancode

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

I said it in the meeting today: I think there are (imo) pretty high value in not having to repeat the same thing all over the place, so I think it's important to also consider moving to having an app-level super class so that these kind of things can have a proper home.

@ef4

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

I like the idea of an app-level base class. Living in the app makes it more search-discoverable by app authors, when compared with imperative injections inside addon code.

@rtablada

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

@chancancode so you're thinking there would be something like app/_controller.js in the blueprint and then users would import and extend that controller (route, component, etc)?

I'd be 👍 for that since it gives a nice inheritance to follow (for humans and tools) while reducing duplication and showing off how to extend common functionality.

@tleperou

This comment has been minimized.

Copy link

commented Jul 4, 2019

Explicitness would make it easier to teach. Otherwise, it can provide a frustrating experience for newbies.

would part of this RFC also include updating the route/controller/etc blueprints to include things like
Let's propose an interactive CLI: do you want to consume the router? the store? the session?

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.