-
-
Notifications
You must be signed in to change notification settings - Fork 68
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
Implicit injection deprecations for v4.0.0 #932
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
--- | ||
id: implicit-injections | ||
title: "Implicit Injections" | ||
until: '5.0.0' | ||
since: '4.0.0' | ||
--- | ||
|
||
Implicit injections have been [deprecated](https://deprecations.emberjs.com/v3.x#toc_implicit-injections) since Ember v3.26.0. As of v4.0.0, implicit injections do nothing and should be removed based on suggestions in the original deprecation. | ||
|
||
Before: | ||
|
||
```js | ||
export default { | ||
initialize(app) { | ||
app.inject('route', 'store', 'service:store'); | ||
} | ||
} | ||
``` | ||
|
||
```js | ||
import { Route } from '@ember/routing/route'; | ||
|
||
export default class ApplicationRoute extends Route { | ||
model() { | ||
return this.store.findQuery('user', 123); | ||
} | ||
} | ||
``` | ||
|
||
After: | ||
|
||
```js | ||
import { Route } from '@ember/routing/route'; | ||
import { inject as service } from '@ember/service'; | ||
|
||
export default class ApplicationRoute extends Route { | ||
@service store; | ||
|
||
model() { | ||
return this.store.findQuery('user', 123); | ||
} | ||
} | ||
``` | ||
|
||
For a more detailed explanation with additional examples, see the 3.x deprecation [guides](https://deprecations.emberjs.com/v3.x#toc_implicit-injections). |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we provide an example of what to remove here?
I think this description is needlessly confusing for someone stumbling into this deprecation without any context. A certain part of the functionality was deprecated in 3.26, but that is not what is deprecated in 4.0 for removal in 5.0. The behavior deprecated in 3.26 is changed in 4.0 already.
Providing a link back to the old deprecation guide is useful context, but the message here has to be something other than "go read the old guide", as the old guide was targeted for apps upgrading to 4.0 and not 5.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is true. All this most recent PR did was move the deprecation from the object receiving the injection to the inject method itself. The only difference being the method just doesn't work now.
I've done a few of these upgrades in addons and apps and the solution has varied widely (some solutions not included in the original deprecation guide). I want to provide good direction so if we think providing an extensive guide similar to the last one is a good idea, then happy to do it. Effectively, the old guides recommend moving to either
@service
orgetOwner
, both of which seem to be a common fix most of the time.If, for example we just include a simple example moving from
owner.inject
to@service
, then we might not be giving a reader the whole picture of what options you might have to take.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is better after the update. I think the theory in 4.0, however, is that the inject call can be removed because it is always a no-op, right?
Telling people to migrate a usage to explicit injections isn't needed, because those were already deprecated in 3.26 and apps should not have them?
Is the guide as simple as being explicit that "you have code that looks like [example], in 4.0 you should delete calls to the injection API since they no longer do anything. See the 3.26 deprecation for more" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor consideration is there are probably cases where ppl will upgrade from <3.26 (thus didn't hit the deprecation) and those that did hit the deprecation. I like your suggestions though. I feel like we might be hitting a middle ground with the current state of the PR - those that are seeing this deprecation for the first time and those that haven't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snewcomer @mixonic just checking in on this. It does look like maybe the after should just import
service
w/oinject
given it's availability in 4.1?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jaredgalanis I don't think that is wise since the deprecation is prior to 4.1.