Add new rule no-implicit-injections

[rtablada]

In response to our team need and the request from 1178, I've implemented a rule for no-implicit-injections to notify and fix deprecations related to https://deprecations.emberjs.com/v3.x/#toc_implicit-injections

Fixes #1178

[rtablada]

@lin-ll I've updated the test cases to include multiple class examples and converted the error message to use messageId to make the messages more informative by outputting the service name.

[bmish]

Can we be consistent with the requirements for passing service names to ember/no-restricted-service-injections? All our rules should accept service names in the same format and throw an error if the service name is not in the proper format.

eslint-plugin-ember/lib/rules/no-restricted-service-injections.js

Lines 60 to 62 in 96a5111

	assert(
	service.toLowerCase() === service,
	'Service name should be passed in kebab-case (all lower case)'

Based on the input, that rule checks for:

• @service myService
• @service() myService
• @service('myService') propertyName
• myService: service()
• propertyName: service('myService')
• propertyName: service('my-service')
• And also logic and tests for handling nested service names

Ideally, we could share code with that rule like the isInjectedServiceProp() helper for detecting the service.

[bmish]

I don't think this has been addressed yet.

[rtablada]

https://github.com/ember-cli/eslint-plugin-ember/pull/1714/files/#diff-ce4c727cc50236f1ac7ea5af2f03053469b2bff730a010ed6f0c25d29ab97319R90-R93

I added the check for rule behavior as requested and updated the md to reference that service names should be kebab cased. https://github.com/ember-cli/eslint-plugin-ember/pull/1714/files/#diff-ce4c727cc50236f1ac7ea5af2f03053469b2bff730a010ed6f0c25d29ab97319R90-R93

I'm not sure I follow what you're saying in the bullet list and I'm unclear on isInjectedServiceProp comment. I based things off the work in no-assigment-of-untracked and the patterns there since I found it to most closely resemble the state checks required to look for uses.

[bmish]

The part about the input format for service names has been addressed.

But I still want to make sure every format of service name mentioned above is able to be detected by your rule when it checks if a service injection already exists. Can you add a test case checking for all possible formats of service injections (including nested service names) to ensure the rule is able to detect each one? You could just pass in ~10 service names to the denylist for one test case and then inject each one in a different format to ensure it's detected.

I was just suggesting isInjectedServiceProp since it could be helpful in regards to checking for service injections and handling the different formats, but not required if you already have a good way to do it.

[rtablada]

The service definition types aren't allowed in the deny list. The deny list config has to be the service module path.

I did however add tests for ways service could be declared in a class and added/fixed use in legacy .extends modules

[rtablada]

Though...

Now I'm realizing that service alone isn't really 100% coverage for uses of auto-injected services.

The current config and code assumes a 1:1 mapping (👋🏽 around kebab and camelCasing) of the injected property name to the service name. But... This may not be the case: initializers can inject any property name they want:

// Example of 1:1 mapping
application.inject('controller', 'flashMessages', 'service:flash-messages');

// Example of arbitrary mapping
application.inject('controller', 'somethingElse', 'service:flash-messages');

I think the config feels good for the 80% usecase (and TBH most teams will likely not configure at all and just look for implicit use of store.

We could augment this with a config propertyName which would default to camelCase(config.service)

[bmish]

If we care about that edge case at all (maybe not?), we can worry about it as a follow-up. I'm ready to merge this PR as soon as I am sure nested services are handled.

[rtablada]

The thought of propertyName came from the issue with nested services`. Ember AFAIK doesn't have a way of auto mapping a property to a nested service.

So if you have an autoInjection of @service('cart/checkout') there has to be an explicit propertyName. I'll add an assertion to check for this config.

[bmish]

Okay. I would still like to see a test with the nested service name.

[rtablada]

Updated and added the assertion mentioned above

[rtablada]

@bmish I think I've addressed the feedback so far.

[bmish]

Can we use required for the denyList property too?

[bmish]

I don't think this has been addressed yet.

[rtablada]

@bmish updated, added tests to make it work in legacy .extends modules as well.

[rtablada]

Alright! Got propertyName config working and updated docs. That actually cleared up some of the confusion and weird naming in the rule.

I think it's now at a point where it works for flexible config, works for legacy modules, AND the code for the rule is easier to read.

[bmish]

Thanks for your great work on this!