Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
#1714Add new rule
no-implicit-injections
#1714Changes from 3 commits
6a50078
ad910a9
1e98255
86568f8
62d9876
e8baa1d
238c372
595c5e5
c1a1cb1
62ac942
2b28afe
c81f3f8
dadc34d
30aea4e
0ea5d4e
5bf714f
05434d8
89d5940
a66d1ea
919ce52
61ebd4f
23b0e1a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
Based on the input, that rule checks for:
@service myService
@service() myService
@service('myService') propertyName
myService: service()
propertyName: service('myService')
propertyName: service('my-service')
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.
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 inno-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
modulesThere 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:
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 tocamelCase(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 explicitpropertyName
. 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