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

forbid overriding builtin services #1347

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NullVoxPopuli
Copy link
Contributor

do folks want this? should I keep working on it?

I think it could be useful, because things only go wrong when people override the router service

@bmish
Copy link
Member

bmish commented Oct 25, 2021

We could call this no-overriding-builtin-services? I would include router and store (Ember Data) for sure. Are there any other builtin services we should include?

I don't think this rule needs to be configurable, since people can use the existing no-restricted-service-injections rule if they want customization.

UPDATE: see other comment thread for more discussion.

valid: [
{
// Service name doesn't match (with property name):
code: `owner.register('service:foo')`,
Copy link
Member

@bmish bmish Oct 26, 2021

Choose a reason for hiding this comment

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

Is owner.register('service:foo') ever used outside tests? I thought it was mostly for tests but I could be wrong. If it's just for tests, do we care about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it technically could happen -- I imagine the behavior would mostly be undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know the speed of certain rules is important, and only looking at test files is a perf improvement we can make.

Maybe it's rare enough where we don't need to worry about app/addon trees?
I'm going to check ember-observer.

In any case, I do still want to prevent this:

let owner = this.owner;

owner.register('...');

this addon will need to keep track of all the ways people can get and manipulate the owner reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@NullVoxPopuli NullVoxPopuli Oct 26, 2021

Choose a reason for hiding this comment

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

a bit more precise: only 12 usages: https://emberobserver.com/code-search?codeQuery=owner%5C.register%5C((%27%7C%22)service&fileFilter=addon&regex=true&sort=usages&sortAscending=false
all seem to be addon-test-support, which is fine -- so we can scope to tests. woohoo!

now, looking at the what we actually want to prevent:
https://emberobserver.com/code-search?codeQuery=owner%5C.register%5C((%27%7C%22)service%3Arouter&fileFilter=tests&regex=true&sort=usages&sortAscending=false

only 21 usages -- less than what I've seen internally 🙃
if only I could check app code with ember observer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the router is the only one I've ran in to. I don't use ember-data regularly enough to know if the store would cause problems 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Okay. So this doesn't seem super useful to me personally but if you feel strongly that it's useful and likely to help people, feel free to proceed.

If you want to proceed, the path forward that I'm leaning towards is:

  1. Create no-router-service-registration that could eventually be enabled as recommended. Limited to just the router service since it's unclear if any other services have a problem with this. If you determine this is likely to apply to other built-in services, then it could be named no-built-in-service-registrations.
  2. If people actually wanted restrict arbitrary service registrations, someone could create an optional no-restricted-service-registrations rule at a later point. Still, I'm unclear that this is actually needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as a library author, i think optionally preventing registration of similar services is important.

i lean towards "no-restricted-service-registrations" with router as default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(i'd use that a bunch internally)

Copy link
Member

@bmish bmish Oct 26, 2021

Choose a reason for hiding this comment

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

i lean towards "no-restricted-service-registrations" with router as default

Okay, I'm open to considering that.

I am just a bit worried about that option as there are many other no-restricted-* rules that are typically intended as purely optional (not recommended) rules which don't restrict anything by default but can be configured to restrict things as desired:

So it may be confusing to people that this new no-restricted-service-registrations rule restricts router by default.

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.

2 participants