Deprecate Implicit Injection on arbitrary Ember Framework objects

[snewcomer]

Rendered

close #508

[rwjblue]

Thanks for taking this on @snewcomer! I'm very much in favor!

[rwjblue]

Can you update with the PR link?

[rwjblue]

I think we need to explain a bit more about why we need multiple phases here (and in each phase).

At the summary level, we should explain that it is impossible for ember-data to remove its owner.inject calls without being a breaking change (that could not have issued a deprecation for, since we couldn't tell if you actually used it).

[rwjblue]

Should mention why we deprecate this first, and how you avoid the deprecation. Specifically, this would allow users to add @service store; explicitly as a way to prevent the deprecation and avoid relying on the implicit injection.

[rwjblue]

Should mention how folks (like ember-data) should mitigate this deprecation. One possible answer is that ember-data can't remove its owner.inject until after ember-data@4. This ordering is a bit nebulous at the moment...

[runspired]

FTR we eagerly inject only because of defaultStore. Were defaultStore not present we would deprecate this ourselves. Deprecating implicit injection without deprecating defaultStore is surprising to me, I'm hoping there was a follow up RFC merged that I've missed 😅

[snewcomer]

I haven't seen DS.defaultStore anywhere for quite a while. Does it still exist?

[runspired]

@snewcomer it's not an EmberData concept, it's an Ember concept. You even linked my issue for needing the framework to remove it in the RFC but I think you possibly misunderstood it as something that we (EmberData) needed to remove. It is currently located here in Ember's codebase: https://github.com/emberjs/ember.js/blob/master/packages/%40ember/-internals/routing/lib/system/route.ts#L2323

[pzuraq]

@runspired is I believe referring to this computed property: https://github.com/emberjs/ember.js/blob/master/packages/@ember/-internals/routing/lib/system/route.ts#L2319

We can and should also deprecate that property, though I think it will have to be for the 5.0 cycle. The deprecation warning should be enough to let user's know that they are using the deprecated functionality, and should explicitly inject @store, I think.

[runspired]

@pzuraq the issue is that if we don't eagerly inject ourself the find call will not be the same as having called ember-data's store.find and results in either this assert triggering or (for folks that happen to have a find method on their model) a very confusing and unexpected request triggering.

assert(
            `${classify(name)} has no method \`find\`.`,
            typeof modelClass.find === 'function'
          );

[runspired]

For folks that were previously making use of EmberData's injected store, they will get the deprecation and hopefully convert to using explicit service injection. For new apps in 4.0 and for places where devs simply forget to add in the explicit inject though the confusion will remain.

[pzuraq]

Right, exactly. I don't think this will be a huge issue as long as we add a deprecation that says loudly in the console:

You are using the deprecated store computed property. This computed property is not a service, and it has confusing and usually unexpected behavior. If you are using Ember Data or another addon that provides a store service, you should add @service('store') to your route in order to get access to the store service instead.

I do agree ideally that the deprecation should be sooner rather than later ideally, but we'll have to see where it fits in the timeline.

[jelhan]

When deprecating default store we should deprecate implicit record loading in default model hook as well. There is a open RFC to do so #557.

[rwjblue]

I agree that considering the phases is important, can you update to indicate relative timing (and likely "triggers")? As it stands now, we say that the timing is super important but don't actually say what the timing is or should be 😆

[BobrImperator]

On a side note, I may be wrong here because of some misunderstanding or sth.

But I remember that there used to be some feature (that i have never relied on) where an implicit route would automagically load a model, is this a thing or is this something i just made up?

The question is, if it exists, how its going to be deprecated?

[snewcomer]

Is this what you are referring to?

#557
#377

[BobrImperator]

Looks like it is it, thank you @snewcomer.
Was wondering how these behaviors play out in this RFC

[snewcomer]

As for the triggers, how does this sound?

1. Introduce deprecation on target
2. Next release introduce deprecation on owner.inject
3. Simmer until v4?

[rwjblue]

IMHO, yes.

[BobrImperator]

I misplaced my comment, so I'll quote it here.

#680 (comment)

On a side note, I may be wrong here because of some misunderstanding or sth.

But I remember that there used to be some feature (that i have never relied on) where an implicit route would automagically load a model, is this a thing or is this something i just made up?

The question is, if it exists, how its going to be deprecated?

[xg-wang]

One use case I have that does not seem easy to migrate is to inject a service that registers global route events to route:application in an initializer. This will eagerly instrument all route transitions without lookup the service in an instance-initializer, otherwise, I cannot mock the service in the test setup.

[jelhan]

One use case I have that does not seem easy to migrate is to inject a service that registers global route events to route:application in an initializer. This will eagerly instrument all route transitions without lookup the service in an instance-initializer, otherwise, I cannot mock the service in the test setup.

Not sure if I fully got it. But can't it be done using RouterService.willChange and RouterService.didChange events? You wouldn't need to inject anything to ApplicationRoute in that case.

[xg-wang]

@jelhan I forgot to mention the use case if for an addon to provide a service that is aware of global router states. The service from the addon does use this.router.on('routeDidChange') in its constructor but services are lazily created on the first lookup.

[jelhan]

@jelhan I forgot to mention the use case if for an addon to provide a service that is aware of global router states. The service from the addon does use this.router.on('routeDidChange') in its constructor but services are lazily created on the first lookup.

Still not getting why you would need to inject the service in application route. Couldn't you call a setup method of the service in instance initializer directly?

[pzuraq]

@xg-wang I’m not sure that this particular deprecation would affect that, because it should still be possible to lookup services with owner.lookup(). So it would still be possible for an initializer to lookup the service and set up some event handlers, if that’s what you’re describing.

If not, can you add some code examples of the case you’re concerned about in order to clarify?

[rwjblue]

If not, can you add some code examples of the case you’re concerned about in order to clarify?

@xg-wang - Maybe throw together a demo repo for us? I'm also unsure that this RFC directly affects your scenario, but without understanding the scenario better I can't be sure...

[xg-wang]

@jelhan @pzuraq @rwjblue apology for the late response. I created https://github.com/xg-wang/test--router-injection-addon to explain what I meant.

The addon will need to prepare initial states, I have a setupMyAddonTest style (setupTest) test helper to fake. However, the this.owner.register won't work if an instance-initializer looks up the service.

Being injected into the main route, the service gets access to all application route transitions.

I hope the demo makes sense, or I can explain more. It's totally possible I was doing things hacky and silly here, please let me know if there's a better way for the use case.

[xg-wang]

Talked to @pzuraq offline, the recommendation for this timing requirement in my demo addon is to look up the service in an instance-initializer and patch the service instance's methods in the test helper, instead of re-registering the class.

[rwjblue]

We discussed this at todays team meeting, and are moving into final comment period.