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
Add new rule no-implicit-injections
#1714
Conversation
chore: generate docs wip: setup for inital create Implement basic identification for implicit injections fix: failure to recognize module type or all modules feat: add fixing for no-implicit-injections update readme for no-implicit-injections
d69eb06
to
6a50078
Compare
no-implicit-injections
@lin-ll I've updated the test cases to include multiple class examples and converted the error message to use |
d06fe3f
to
595c5e5
Compare
9dbca60
to
62ac942
Compare
docs/rules/no-implicit-injections.md
Outdated
|
||
- object -- containing the following properties: | ||
- array -- `services` -- Array of configuration objects configuring the lint rule to check for use of implicit injected services | ||
- string -- `serviceName` -- The name of the service that is implicitly injected |
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 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.
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.
I don't think this has been addressed yet.
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.
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.
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.
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.
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.
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
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.
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)
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.
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.
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.
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.
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.
Okay. I would still like to see a test with the nested service name.
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.
Updated and added the assertion mentioned above
@bmish I think I've addressed the feedback so far. |
{ | ||
type: 'object', | ||
properties: { | ||
denyList: { |
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 use required
for the denyList
property too?
docs/rules/no-implicit-injections.md
Outdated
|
||
- object -- containing the following properties: | ||
- array -- `services` -- Array of configuration objects configuring the lint rule to check for use of implicit injected services | ||
- string -- `serviceName` -- The name of the service that is implicitly injected |
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.
I don't think this has been addressed yet.
Co-authored-by: Bryan Mishkin <698306+bmish@users.noreply.github.com>
@bmish updated, added tests to make it work in legacy |
Alright! Got 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. |
d96fd53
to
1c4a0fd
Compare
1c4a0fd
to
23b0e1a
Compare
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.
Thanks for your great work on this!
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-injectionsFixes #1178