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.

Sign up for GitHub

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

Jump to bottom

refactor: avoid using file name to determine class identity. #603

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

refactor: avoid using file name to determine class identity. #603

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a85b09a
refactor: move module name helper into separate file
alexlafroscia Nov 21, 2019
d788c25
refactor: extract utility for getting import node
alexlafroscia Nov 21, 2019
d929f5e
feat: add helper for checking if an Identifier is a default export
alexlafroscia Nov 21, 2019
39e3036
chore: update test cases to include imports
alexlafroscia Nov 21, 2019
535cb40
refactor: avoid using file name in rules
alexlafroscia Nov 21, 2019
ba714c5
test(require-tagless-components): add Service tests
alexlafroscia Nov 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion lib/rules/alias-model-in-controller.js
View file
Open in desktop
Expand Up @@ -29,7 +29,9 @@ module.exports = {
};

return {
CallExpression(node) {
'CallExpression > MemberExpression[property.name="extend"]'(memberExpressionNode) {
Copy link
Member

Choose a reason for hiding this comment

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

We should not push this complexity (checking for extends and parents and such) into each rule. Each rule should be able to make one simple util function call to check if the node is an Ember controller/component/etc, like this:

CallExpression(node) {
  // classic class
  if (ember.isEmberController(context, node)) {
    ...
  }
},

ClassDeclaration(node) {
  // native class
  if (ember.isEmberController(context, node)) {
    ...
  }
},

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'm not sure I agree -- it seems like we have a specific CallExpression pattern we're looking for, which looks like

Foo.extend()

We have the ability to tell ESLint those details -- why would we want to check every call expression if we don't have to?

Copy link
Member

Choose a reason for hiding this comment

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

To my knowledge, there's no actual difference between using the more-specific selector vs. doing it ourselves: either eslint does the checks or we do the checks. In general, I would agree with you that it's nice to take advantage of using more-specific selectors, but I don't think that's appropriate here, because:

  1. Using both the selector matching and util function unnecessarily splits the logic for detecting components/controllers/etc into two separate places instead of consolidating the logic fully inside a single util function (and maybe this util function could even come from a third-party library someday).
  2. Using the selector matching duplicates part of the detection logic across countless rules, which increases the chance for typos and inconsistencies between rules.
  3. Encapsulating all of the logic in a util function makes it easy to unit test it in its entirety.
  4. The rule code is simpler when the detection details are delegated to the util function. We should optimize for simplicity and readability, and make it as easy to write rules as possible (which includes providing extremely simple util function APIs).

const node = memberExpressionNode.parent;

if (!ember.isEmberController(context, node)) {
return;
}
Expand Down
31 changes: 18 additions & 13 deletions lib/rules/avoid-using-needs-in-controllers.js
View file
Open in desktop
Expand Up @@ -27,22 +27,27 @@ module.exports = {
context.report(node, message);
};

return {
CallExpression(node) {
const isReopenNode = ember.isReopenObject(node) || ember.isReopenClassObject(node);
function checkMemberExpression(memberExpressionNode) {
const node = memberExpressionNode.parent;

if (!ember.isEmberController(context, node) && !isReopenNode) {
return;
}
if (!ember.isEmberController(context, node)) {
return;
}

const properties = ember.getModuleProperties(node);
const properties = ember.getModuleProperties(node);

properties.forEach(property => {
if (property.key.name === 'needs') {
report(property);
}
});
},
properties.forEach(property => {
if (property.key.name === 'needs') {
report(property);
}
});
}

return {
'CallExpression > MemberExpression[property.name="extend"]': checkMemberExpression,
'CallExpression > MemberExpression[property.name="reopen"]': checkMemberExpression,
'CallExpression > MemberExpression[property.name="reopenClass"]': checkMemberExpression,
'CallExpression > MemberExpression[property.value="extend"]': checkMemberExpression,
};
},
};
2 changes: 1 addition & 1 deletion lib/rules/new-module-imports.js
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@

const MAPPING = require('ember-rfc176-data');
const { buildMessage, getFullNames, isDestructuring } = require('../utils/new-module');
const { getSourceModuleNameForIdentifier } = require('../utils/utils');
const { getSourceModuleNameForIdentifier } = require('../utils/import-info');

const GLOBALS = MAPPING.reduce((memo, exportDefinition) => {
if (exportDefinition.deprecated) {
Expand Down
4 changes: 3 additions & 1 deletion lib/rules/no-actions-hash.js
View file
Open in desktop
Expand Up @@ -41,7 +41,9 @@ module.exports = {
reportActionsProp(utils.findNodes(node.body.body, 'ClassProperty'));
}
},
CallExpression(node) {
'CallExpression > MemberExpression[property.name="extend"]'(memberExpressionNode) {
const node = memberExpressionNode.parent;

if (inClassWhichCanContainActions(context, node)) {
reportActionsProp(ember.getModuleProperties(node));
}
Expand Down
2 changes: 1 addition & 1 deletion lib/rules/no-classic-classes.js
View file
Open in desktop
@@ -1,6 +1,6 @@
'use strict';

const { getSourceModuleNameForIdentifier } = require('../utils/utils');
const { getSourceModuleNameForIdentifier } = require('../utils/import-info');
const { isObjectExpression } = require('../utils/types');

const ERROR_MESSAGE_NO_CLASSIC_CLASSES =
Expand Down
7 changes: 5 additions & 2 deletions lib/rules/no-get.js
View file
Open in desktop
Expand Up @@ -84,10 +84,13 @@ module.exports = {
// Check for situations which the rule should ignore.
// **************************

if (emberUtils.isEmberProxy(context, node)) {
if (emberUtils.isEmberObject(node) && emberUtils.isEmberProxy(context, node)) {
currentProxyObject = node; // Keep track of being inside a proxy object.
}
if (emberUtils.isEmberObjectImplementingUnknownProperty(node)) {
if (
emberUtils.isEmberObject(node) &&
emberUtils.isEmberObjectImplementingUnknownProperty(node)
) {
currentClassWithUnknownPropertyMethod = node;
}
if (currentProxyObject || currentClassWithUnknownPropertyMethod) {
Expand Down
4 changes: 3 additions & 1 deletion lib/rules/no-new-mixins.js
View file
Open in desktop
Expand Up @@ -26,7 +26,9 @@ module.exports = {

create(context) {
return {
CallExpression(node) {
'CallExpression > MemberExpression[property.name="create"]'(memberExpressionNode) {
const node = memberExpressionNode.parent;

if (ember.isEmberMixin(context, node)) {
context.report(node, ERROR_MESSAGE);
}
Expand Down
4 changes: 3 additions & 1 deletion lib/rules/no-on-calls-in-components.js
View file
Open in desktop
Expand Up @@ -63,7 +63,9 @@ module.exports = {
};

return {
CallExpression(node) {
'CallExpression > MemberExpression[property.name="extend"]'(memberExpressionNode) {
const node = memberExpressionNode.parent;

if (!ember.isEmberComponent(context, node)) {
return;
}
Expand Down
4 changes: 3 additions & 1 deletion lib/rules/order-in-components.js
View file
Open in desktop
Expand Up @@ -84,7 +84,9 @@ module.exports = {
}

return {
CallExpression(node) {
'CallExpression > MemberExpression[property.name="extend"]'(memberExpressionNode) {
const node = memberExpressionNode.parent;

if (!ember.isEmberComponent(context, node)) {
return;
}
Expand Down
4 changes: 3 additions & 1 deletion lib/rules/order-in-controllers.js
View file
Open in desktop
Expand Up @@ -56,7 +56,9 @@ module.exports = {
: ORDER;

return {
CallExpression(node) {
'CallExpression > MemberExpression[property.name="extend"]'(memberExpressionNode) {
const node = memberExpressionNode.parent;

if (!ember.isEmberController(context, node)) {
return;
}
Expand Down
4 changes: 3 additions & 1 deletion lib/rules/order-in-models.js
View file
Open in desktop
Expand Up @@ -46,7 +46,9 @@ module.exports = {
const filePath = context.getFilename();

return {
CallExpression(node) {
'CallExpression > MemberExpression[property.name="extend"]'(memberExpressionNode) {
const node = memberExpressionNode.parent;

if (!ember.isDSModel(node, filePath)) {
return;
}
Expand Down
4 changes: 3 additions & 1 deletion lib/rules/order-in-routes.js
View file
Open in desktop
Expand Up @@ -79,7 +79,9 @@ module.exports = {
}

return {
CallExpression(node) {
'CallExpression > MemberExpression[property.name="extend"]'(memberExpressionNode) {
const node = memberExpressionNode.parent;

if (!ember.isEmberRoute(context, node)) {
return;
}
Expand Down
4 changes: 3 additions & 1 deletion lib/rules/require-super-in-init.js
View file
Open in desktop
Expand Up @@ -88,7 +88,9 @@ module.exports = {
};

return {
CallExpression(node) {
'CallExpression > MemberExpression[property.name="extend"]'(memberExpressionNode) {
const node = memberExpressionNode.parent;

if (
!ember.isEmberComponent(context, node) &&
!ember.isEmberController(context, node) &&
Expand Down
2 changes: 1 addition & 1 deletion lib/rules/require-tagless-components.js
View file
Open in desktop
@@ -1,7 +1,7 @@
'use strict';

const { isEmberComponent } = require('../utils/ember');
const { getSourceModuleNameForIdentifier } = require('../utils/utils');
const { getSourceModuleNameForIdentifier } = require('../utils/import-info');
const {
isCallExpression,
isIdentifier,
Expand Down
2 changes: 1 addition & 1 deletion lib/rules/use-ember-data-rfc-395-imports.js
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@

const MAPPINGS = require('@ember-data/rfc395-data');
const { buildFix, buildMessage, getFullNames, isDestructuring } = require('../utils/new-module');
const { getSourceModuleNameForIdentifier } = require('../utils/utils');
const { getSourceModuleNameForIdentifier } = require('../utils/import-info');

/**
* This function returns an object like this:
Expand Down
47 changes: 26 additions & 21 deletions lib/utils/ember.js
View file
Open in desktop
@@ -1,5 +1,6 @@
'use strict';

const { getSourceModuleNameForIdentifier, isDefaultImport } = require('./import-info');
const utils = require('./utils');
const types = require('./types');
const assert = require('assert');
Expand Down Expand Up @@ -70,17 +71,6 @@ const CORE_MODULE_IMPORT_PATHS = {
ObjectProxy: '@ember/object/proxy',
};

function isClassicEmberCoreModule(node, module, filePath) {
const isExtended = isEmberObject(node);
let isModuleByPath;

if (filePath) {
isModuleByPath = isModuleByFilePath(filePath, module.toLowerCase()) && isExtended;
}

return isModule(node, module) || isModuleByPath;
}

function isLocalModule(callee, element) {
return (
(types.isIdentifier(callee) && callee.name === element) ||
Expand Down Expand Up @@ -169,26 +159,41 @@ function isReopenObject(node) {
}

function isEmberCoreModule(context, node, moduleName) {
// Handle Ember.X references to classes
if (
types.isCallExpression(node) &&
types.isMemberExpression(node.callee) &&
types.isMemberExpression(node.callee.object) &&
utils.getSourceModuleName(node.callee) === 'Ember'
) {
return node.callee.object.property.name === moduleName;
}

let superClass;

if (types.isCallExpression(node)) {
// "classic" class pattern
return isClassicEmberCoreModule(node, moduleName, context.getFilename());
superClass = node.callee.object;
} else if (types.isClassDeclaration(node)) {
// native classes
if (!node.superClass) {
return false;
}
const superClassImportPath = utils.getSourceModuleNameForIdentifier(context, node.superClass);

if (superClassImportPath === CORE_MODULE_IMPORT_PATHS[moduleName]) {
return true;
}
superClass = node.superClass;
} else {
assert(
false,
'Function should only be called on a `CallExpression` (classic class) or `ClassDeclaration` (native class)'
);
}
return false;

if (!superClass) {
return false;
}

const superClassImportPath = getSourceModuleNameForIdentifier(context, superClass);

return (
superClassImportPath === CORE_MODULE_IMPORT_PATHS[moduleName] &&
isDefaultImport(context, superClass)
);
}

function isEmberComponent(context, node) {
Expand Down
66 changes: 66 additions & 0 deletions lib/utils/import-info.js
View file
Open in desktop
@@ -0,0 +1,66 @@
'use strict';

const { isImportDeclaration, isImportDefaultSpecifier } = require('./types');
const { getSourceModuleName } = require('./utils');

module.exports = {
getSourceModuleNameForIdentifier,
isDefaultImport,
};

/**
* Gets the ImportDeclaration node that an identifier was imported from
* if it was imported
*
* @param {Object} context the context of the ESLint rule
* @param {node} node the Idenfifier to find an import for
* @returns {ImportDeclaration | undefined} The ImportDeclaration of the module the identifier was imported from, if it was imported
*/
function getImportDeclarationNode(context, node) {
const [program] = context.getAncestors(node);

return program.body
.filter(isImportDeclaration)
.find(importDeclaration =>
importDeclaration.specifiers.some(
specifier => specifier.local.name === getSourceModuleName(node)
)
);
}

function getImportSpecifierNode(context, node) {
const importDeclaration = getImportDeclarationNode(context, node);

return importDeclaration
? importDeclaration.specifiers.find(
specifier => specifier.local.name === getSourceModuleName(node)
)
: undefined;
}

/**
* Gets the name of the module that an identifier was imported from,
* if it was imported
*
* @param {Object} context the context of the ESLint rule
* @param {node} node the Idenfifier to find an import for
* @returns {string | undefined} The name of the module the identifier was imported from, if it was imported
*/
function getSourceModuleNameForIdentifier(context, node) {
const importDeclaration = getImportDeclarationNode(context, node);

return importDeclaration ? importDeclaration.source.value : undefined;
}

/**
* Determines whether the given Identifer was the default import from some module
*
* @param {Object} context the context of the ESLint rule
* @param {node} node the Idenfifier to check
* @returns {boolean} Whether or not the identifier is a default import from a module
*/
function isDefaultImport(context, node) {
const importSpecifier = getImportSpecifierNode(context, node);

return isImportDefaultSpecifier(importSpecifier);
}
11 changes: 11 additions & 0 deletions lib/utils/types.js
View file
Open in desktop
Expand Up @@ -18,6 +18,7 @@ module.exports = {
isFunctionExpression,
isIdentifier,
isImportDeclaration,
isImportDefaultSpecifier,
isLiteral,
isLogicalExpression,
isMemberExpression,
Expand Down Expand Up @@ -225,6 +226,16 @@ function isImportDeclaration(node) {
return node !== undefined && node.type === 'ImportDeclaration';
}

/**
* Check whether or not a node is an ImportDefaultSpecifier.
*
* @param {Object} node The node to check.
* @returns {boolean} Whether or not the node is an ImportDefaultSpecifier.
*/
function isImportDefaultSpecifier(node) {
return node !== undefined && node.type === 'ImportDefaultSpecifier';
}

/**
* Check whether or not a node is an Literal.
*
Expand Down