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

New rule: no-implicit-service-injection-argument #195

Closed
knownasilya opened this issue Dec 16, 2017 · 8 comments · Fixed by #1188
Closed

New rule: no-implicit-service-injection-argument #195

knownasilya opened this issue Dec 16, 2017 · 8 comments · Fixed by #1188
Labels
enhancement New Rule Idea for a new lint rule

Comments

@knownasilya
Copy link
Contributor

knownasilya commented Dec 16, 2017

I'm proposing adding a new Best Practices rule that promotes usage of explicit injections.

So instead of:

store: inject.service()

The recommended way would be:

store: inject.service('store')

The reasoning behind this change, is that I've heard new Ember developers asking about what that means and where is the file that defines the given injection. It's a sort of "black magic" that allows for shortcuts that don't really help down the line. For example:

users: inject.controller()
// The route also defines `users` as a model.

Now if a new developer saw that in the template, even if there wasn't a model defined, they might think it's the model and try to do {{#each users as |user|}}.

A better approach:

usersController: inject.controller('users')
// The route defines `users` as a model.

The above won't prevent users from naming injections poorly, but it will allow them to be more verbose by default.

This rule could have a fix as well, and would open up the way to deprecating the implicit injections all-together, via an RFC. I'm sure that there are also performance improvements when you no longer have to do that implicit check in the internals.

@kellyselden
Copy link
Member

I would love to have this rule, but I don't know about adding to an "on by default" group to start.

@piotrpalek
Copy link

I would personally be against this rule, especially as a default. In my experience people tend to name the same services differently in every component / controller / etc which makes searching for those services tedious.
The opposite way (recommending to use users: inject.controller()) would make the code more consistent and in result easier to parse.
It's definitely a tradeoff, but in the spirit of guiding users into the "pit of success" I'd say this approach is a clear winner since long-term the codebase stays more consistent (with the tradeoff being that it can be a bit confusing initially).

@knownasilya
Copy link
Contributor Author

Sure it requires consistency in a way, but its hard to follow in the template or elsewhere that you are working with a service. I think this probably won't be a default rule since most people do it the other way.

The reason I've submitted this is to help with teams that prefer the more explicit method and aren't used to this kind of "magic".

@knownasilya
Copy link
Contributor Author

Give it a test here #196

@bmish
Copy link
Member

bmish commented Aug 4, 2019

Related rule: no-unnecessary-service-injection-argument

Copy link
Member

rwjblue commented Aug 5, 2019

FWIW, I would love if this rule existed.

@knownasilya
Copy link
Contributor Author

I'll try to get it across the finish line this week.

@bmish bmish changed the title New rule: no-implicit-injections New rule: no-implicit-service-injection-argument May 9, 2021
@bmish
Copy link
Member

bmish commented May 9, 2021

I opened a PR to implement no-implicit-service-injection-argument: #1188

@bmish bmish added the New Rule Idea for a new lint rule label May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New Rule Idea for a new lint rule
Projects
None yet
6 participants