-
-
Notifications
You must be signed in to change notification settings - Fork 204
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
NullVoxPopuli
wants to merge
1
commit into
ember-cli:master
Choose a base branch
from
NullVoxPopuli:forbid-overriding-builtin-services
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,121 @@ | ||
'use strict'; | ||
|
||
const assert = require('assert'); | ||
const emberUtils = require('../utils/ember'); | ||
const {getImportIdentifier} = require('../utils/import'); | ||
|
||
const DEFAULT_ERROR_MESSAGE = 'Registering this service is not allowed'; | ||
const DEFAULT_RESTRICTED_REGISTRATIONS = ['router']; | ||
|
||
/** | ||
* @type {import('eslint').Rule.RuleModule} | ||
*/ | ||
module.exports = { | ||
meta: { | ||
type: 'suggestion', | ||
docs: { | ||
description: 'disallow registering certain services under certain paths', | ||
category: 'Services', | ||
url: 'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/no-restricted-service-registrations.md', | ||
}, | ||
schema: { | ||
type: 'array', | ||
uniqueItems: true, | ||
minItems: 1, | ||
items: [ | ||
{ | ||
type: 'object', | ||
required: ['services'], | ||
properties: { | ||
services: { | ||
type: 'array', | ||
uniqueItems: true, | ||
minItems: 1, | ||
items: { | ||
type: 'string', | ||
}, | ||
}, | ||
message: { | ||
type: 'string', | ||
}, | ||
}, | ||
additionalProperties: false, | ||
}, | ||
], | ||
}, | ||
}, | ||
|
||
DEFAULT_ERROR_MESSAGE, | ||
|
||
create(context) { | ||
if (!emberUtils.isTestFile(context.getFilename())) { | ||
return {}; | ||
} | ||
|
||
// Validate options. | ||
for (const option of context.options) { | ||
for (const service of option.services) { | ||
assert( | ||
service.toLowerCase() === service, | ||
'Service name should be passed in kebab-case (all lower case)' | ||
); | ||
} | ||
} | ||
|
||
function checkForViolationAndReport(node, serviceName) { | ||
const serviceNameKebabCase = emberUtils.convertServiceNameToKebabCase(serviceName); // splitting is used to avoid converting folder/ to folder- | ||
|
||
for (const denylist of denylists) { | ||
// Denylist services are always passed in in kebab-case, so we can do a kebab-case comparison. | ||
if (denylist.services.includes(serviceNameKebabCase)) { | ||
context.report({ | ||
node, | ||
message: denylist.message || DEFAULT_ERROR_MESSAGE, | ||
}); | ||
} | ||
} | ||
} | ||
|
||
let getContextName; | ||
let potentials = []; | ||
|
||
return { | ||
ImportDeclaration(node) { | ||
if (node.source.value === '@ember/test-helpers') { | ||
getContextName = getContextName || getImportIdentifier(node, 'getContext'); | ||
} | ||
}, | ||
|
||
MemberExpression(node) { | ||
if (node.object.name === 'owner' && node.property.name === 'register') { | ||
potentials.push(node); | ||
} | ||
|
||
}, | ||
|
||
CallExpression(node) { | ||
potentials = []; | ||
}, | ||
|
||
'CallExpression:exit'(node) { | ||
for (let potential of potentials) { | ||
let first = node.arguments[0].value; | ||
|
||
let [namespace, name] = first.split(':'); | ||
|
||
if (namespace !== 'service') { | ||
return; | ||
} | ||
|
||
if (name === 'router') { | ||
context.report({ | ||
node, | ||
message: `Cannot register the router service`, | ||
}); | ||
} | ||
} | ||
} | ||
|
||
}; | ||
}, | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
const RuleTester = require('eslint').RuleTester; | ||
const rule = require('../../../lib/rules/no-restricted-service-registrations'); | ||
|
||
const { DEFAULT_ERROR_MESSAGE } = rule; | ||
|
||
const ruleTester = new RuleTester({ | ||
parser: require.resolve('@babel/eslint-parser'), | ||
parserOptions: { | ||
ecmaVersion: 2020, | ||
sourceType: 'module', | ||
}, | ||
}); | ||
|
||
ruleTester.run('no-restricted-service-registrations', rule, { | ||
valid: [ | ||
{ | ||
// Service name doesn't match (with property name): | ||
code: `owner.register('service:foo')`, | ||
options: [{ services: ['abc'] }], | ||
}, | ||
{ | ||
// Namespace doesn't match | ||
code: `owner.register('model:abc')`, | ||
options: [{ services: ['abc'] }], | ||
}, | ||
], | ||
invalid: [ | ||
// Basic test usage | ||
{ | ||
code: `this.owner.register('service:router', null)`, | ||
output: null, | ||
errors: [{ message: `Cannot register the router service.` }], | ||
}, | ||
// owner assigned to a variable | ||
{ | ||
code: `let a = this.owner; a.register('service:router', null)`, | ||
output: null, | ||
errors: [{ message: `Cannot register the router service.` }], | ||
}, | ||
// using getContext().owner | ||
{ | ||
code: `import { getContext } from '@ember/test-helpers'; getContext().owner.register('service:router', null)`, | ||
output: null, | ||
errors: [{ message: `Cannot register the router service.` }], | ||
}, | ||
// using getContext, but renamed | ||
{ | ||
code: `import { getContext as a } from '@ember/test-helpers'; a().owner.register('service:router', null)`, | ||
output: null, | ||
errors: [{ message: `Cannot register the router service.` }], | ||
}, | ||
// using { owner } = getContext() | ||
{ | ||
code: `let { owner: a } = getContext(); a.register('service:router', null)`, | ||
output: null, | ||
errors: [{ message: `Cannot register the router service.` }], | ||
}, | ||
], | ||
}); |
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.
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.
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?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.
it technically could happen -- I imagine the behavior would mostly be undefined
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 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:
this addon will need to keep track of all the ways people can get and manipulate the owner reference
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 search: https://emberobserver.com/code-search?codeQuery=owner.register
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.
a bit more precise: only 12 usages: https://emberobserver.com/code-search?codeQuery=owner%5C.register%5C((%27%7C%22)service&fileFilter=addon®ex=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®ex=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
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 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 🤷
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. 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:
no-router-service-registration
that could eventually be enabled asrecommended
. Limited to just therouter
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 namedno-built-in-service-registrations
.no-restricted-service-registrations
rule at a later point. Still, I'm unclear that this is actually needed.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.
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
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'd use that a bunch internally)
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'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 (notrecommended
) 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 restrictsrouter
by default.