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

Implicit injection deprecations for v4.0.0 #932

Merged
merged 3 commits into from Apr 24, 2022

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

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.

None yet

5 participants