Skip to content

Implicit injection deprecations for v4.0.0#932

Merged
locks merged 3 commits intoember-learn:mainfrom
snewcomer:sn/implicit-5.0
Apr 24, 2022
Merged

Implicit injection deprecations for v4.0.0#932
locks merged 3 commits intoember-learn:mainfrom
snewcomer:sn/implicit-5.0

Conversation

@snewcomer
Copy link
Contributor

@snewcomer snewcomer commented Aug 20, 2021

Per discussion at https://github.com/emberjs/ember.js/pull/19680/files#r692596832, add a deprecation guide entry for implicit injection APIs deprecation in 4.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.
Copy link
Contributor

@mixonic mixonic Aug 21, 2021

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.

Copy link
Contributor Author

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 or getOwner, 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.

Copy link
Contributor

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" ?

Copy link
Contributor Author

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.

Copy link
Member

@jaredgalanis jaredgalanis Jan 18, 2022

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/o inject given it's availability in 4.1?

Copy link
Contributor

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.

@bartocc
Copy link
Contributor

bartocc commented Apr 21, 2022

Hey folks, what's the status of this PR? it would be great to have this deprecation added to the guides

@snewcomer
Copy link
Contributor Author

@bartocc agreed. Been close to 9 months now and something as simple as this would have benefitted from being merged (even with further considerations to be made) for folks rather than sitting in limbo 😢. If I could merge, I would have.

@locks locks merged commit 09eecbc into ember-learn:main Apr 24, 2022
@snewcomer snewcomer deleted the sn/implicit-5.0 branch April 24, 2022 13:05
@bartocc
Copy link
Contributor

bartocc commented Apr 25, 2022

Awesome 🎉

Would it be desirable/possible to also update the section Deprecations added in 4.0 in the Ember 4.0 released post?

@locks
Copy link
Contributor

locks commented May 14, 2022

Send a PR over and we can review!

@bartocc
Copy link
Contributor

bartocc commented May 16, 2022

Send a PR over and we can review!

I'll do that 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants