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

Add new rule no-implicit-service-injection-argument #1188

Merged

Conversation

bmish
Copy link
Member

@bmish bmish commented May 9, 2021

Fixes #195.


import Component from '@ember/component';
import { inject as service } from '@ember/service';
export default class Page extends Component {
  @service() serviceName; // Bad

  @service('service-name') serviceName; // Good
  @service('serviceName') serviceName; // Good
}

@bmish bmish force-pushed the no-implicit-service-injection-argument branch from 6fab935 to 772c0f3 Compare May 9, 2021 21:32

Disallow omitting the service name argument when injecting a service.

Some developers may prefer to be more explicit about what service is being injected instead of relying on the implicit and potentially costly lookup/normalization of the service from the property name.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwjblue is this the right justification for this rule?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@runspired thoughts?

Copy link

@runspired runspired May 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's generally the right motivation, though in truth my feelings here have shifted with time.

With classic syntax, the implicit felt extra magical. With native class syntax and decorators, the service name being the key is a lot more clear (and understandable) I feel.

I think at this point what I want is for an error if the service name normalized from the key would be different from resolved name, I'm not sure if we can give the rule enough context for that or not. The key point is to enforce that it would work with the strict-resolver package if desired.

@scalvert or @stefanpenner may have good feedback here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@runspired thanks for the context. I agree with your goal of ensuring the service injection property matches the resolved name to avoid the need for normalization. I'm thinking of proceeding with this plan:

  1. Merge no-implicit-service-injection-argument as-is, always requiring the service injection argument. This would be intended as an optional (non-recommended) rule for people who prefer this style.
    • In the future, we could potentially make it smarter/configurable about when to require the service injection argument.
  2. Create a new rule require-normalized-service-injections (or similar name) which would both validate that the service being injected actually exists, and that the name it's injected with matches its resolved name (so that it works with ember-strict-resolver). This can be implemented once it's practical for the lint rule to understand the full service resolution logic (including how to find services in addons).

import { inject as service } from '@ember/service';

export default class Page extends Component {
@service('service-name') serviceName;
Copy link
Member Author

@bmish bmish May 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwjblue are both @service('service-name') and @service('serviceName') acceptable? Should we allow just one or both? And which one should we autofix to?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

@bmish bmish May 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the rule to autofix to the kebab-case service name (i.e. @service('service-name')) since kebab-case filenames are more common. In the future, once we can actually resolve services from inside the lint rule, we can make this autofixer use the actual resolved service name.

Also see this discussion: #1188 (comment)

@bmish bmish requested a review from rwjblue May 9, 2021 21:34
@bmish bmish force-pushed the no-implicit-service-injection-argument branch from 772c0f3 to aacb5a2 Compare May 9, 2021 21:36
@bmish bmish mentioned this pull request May 9, 2021
4 tasks
@bmish bmish force-pushed the no-implicit-service-injection-argument branch 2 times, most recently from 9e04a16 to 7c57229 Compare May 10, 2021 19:39
@bmish bmish force-pushed the no-implicit-service-injection-argument branch from 5942c09 to 507167f Compare May 29, 2021 16:12
@bmish bmish force-pushed the no-implicit-service-injection-argument branch from 507167f to 16cf66c Compare May 30, 2021 19:38
@bmish bmish merged commit 019746c into ember-cli:master May 30, 2021
@bmish bmish deleted the no-implicit-service-injection-argument branch May 30, 2021 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New rule: no-implicit-service-injection-argument
2 participants