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

Check imports when detecting computed properties in many rules #909

Merged
merged 1 commit into from Aug 9, 2020
Merged

Check imports when detecting computed properties in many rules #909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 18 additions & 1 deletion lib/rules/computed-property-getters.js
View file
Open in desktop
Expand Up @@ -2,6 +2,7 @@

const ember = require('../utils/ember');
const types = require('../utils/types');
const { getImportIdentifier } = require('../utils/import');

//------------------------------------------------------------------------------
// General rule - Prevent using a getter inside computed properties.
Expand Down Expand Up @@ -94,9 +95,25 @@ module.exports = {
}
};

let importedEmberName;
let importedComputedName;
Comment on lines +98 to +99
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, there is nothing wrong with this (from a technical level):

import Ember from 'ember';
import Em from 'ember';
import LOL from 'ember;

All in the same module, and IMHO ^ should still trigger the rule. If I understand this code correctly, we are assuming there is only one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

While dealing with this would be a large hassle to just tack on to the current implementation, I think it would be pretty easy to handle with the gatherImports-style refactor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will consider this in the future, although it does seem like it could be pretty inconvenient to handle this edge case. Hopefully, most people are using a rule like import/no-duplicates to avoid this.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe, not sure. I would expect that the goal of the a higher level gatherImports style API is to abstract this issue away.


return {
ImportDeclaration(node) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This logic is getting repeated a lot of times. It could be nice to make a more generic imports object, and do something like imports = updateImports(imports, node). Then isComputedProp could take that imports object as an argument.

Copy link
Member

Choose a reason for hiding this comment

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

Ya, or even something like:

let imports = new Map();

return {
  ImportDeclaration: gatherImports(imports),

  // ...snip...
}

Where the gatherImports method returns a function that works as a visitor handler itself, or if you also have custom import logic you need you could do this (with the same API):

let imports = new Map();
let importGatherer = gatherImports(imports);

return {
  ImportDeclaration(node) {
    importGatherer(node);
    // other logic
  }
};

Copy link
Member Author

@bmish bmish Aug 8, 2020 •
edited

Choose a reason for hiding this comment

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

Definitely agree with this. I may do this as a follow-up so I can focus on this improved import gathering API by itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Working on the new API in this PR: #910

if (node.source.value === 'ember') {
importedEmberName = importedEmberName || getImportIdentifier(node, 'ember');
}
if (node.source.value === '@ember/object') {
importedComputedName =
importedComputedName || getImportIdentifier(node, '@ember/object', 'computed');
}
},

CallExpression(node) {
if (ember.isComputedProp(node) && node.arguments.length > 0) {
if (
ember.isComputedProp(node, importedEmberName, importedComputedName) &&
node.arguments.length > 0
) {
if (requireGetters === 'always-with-setter') {
requireGetterOnlyWithASetterInComputedProperty(node);
}
Expand Down
18 changes: 17 additions & 1 deletion lib/rules/no-arrow-function-computed-properties.js
View file
Open in desktop
Expand Up @@ -2,6 +2,7 @@

const types = require('../utils/types');
const emberUtils = require('../utils/ember');
const { getImportIdentifier } = require('../utils/import');

const ERROR_MESSAGE = 'Do not use arrow functions in computed properties';

Expand Down Expand Up @@ -37,7 +38,20 @@ module.exports = {

let isThisPresent = false;

let importedEmberName;
let importedComputedName;

return {
ImportDeclaration(node) {
if (node.source.value === 'ember') {
importedEmberName = importedEmberName || getImportIdentifier(node, 'ember');
}
if (node.source.value === '@ember/object') {
importedComputedName =
importedComputedName || getImportIdentifier(node, '@ember/object', 'computed');
}
},

ThisExpression() {
isThisPresent = true;
},
Expand All @@ -46,7 +60,9 @@ module.exports = {
},
'CallExpression:exit'(node) {
const isComputedArrow =
emberUtils.isComputedProp(node) &&
emberUtils.isComputedProp(node, importedEmberName, importedComputedName, {
includeMacro: true,
}) &&
node.arguments.length > 0 &&
types.isArrowFunctionExpression(node.arguments[node.arguments.length - 1]);

Expand Down
19 changes: 15 additions & 4 deletions lib/rules/no-deeply-nested-dependent-keys-with-each.js
View file
Open in desktop
@@ -1,6 +1,7 @@
'use strict';

const emberUtils = require('../utils/ember');
const { getImportIdentifier } = require('../utils/import');

//------------------------------------------------------------------------------
// Rule Definition
Expand All @@ -26,12 +27,22 @@ module.exports = {
ERROR_MESSAGE,

create(context) {
let importedEmberName;
let importedComputedName;

return {
ImportDeclaration(node) {
if (node.source.value === 'ember') {
importedEmberName = importedEmberName || getImportIdentifier(node, 'ember');
}
if (node.source.value === '@ember/object') {
importedComputedName =
importedComputedName || getImportIdentifier(node, '@ember/object', 'computed');
}
},

CallExpression(node) {
if (
emberUtils.isComputedProp(node) &&
(!node.callee.property || node.callee.property.name === 'computed')
) {
if (emberUtils.isComputedProp(node, importedEmberName, importedComputedName)) {
emberUtils.parseDependentKeys(node).forEach((key) => {
const parts = key.split('.');
const indexOfAtEach = parts.indexOf('@each');
Expand Down
16 changes: 15 additions & 1 deletion lib/rules/no-duplicate-dependent-keys.js
View file
Open in desktop
Expand Up @@ -4,6 +4,7 @@ const emberUtils = require('../utils/ember');
const types = require('../utils/types');
const fixerUtils = require('../utils/fixer');
const javascriptUtils = require('../utils/javascript');
const { getImportIdentifier } = require('../utils/import');

const ERROR_MESSAGE = 'Dependent keys should not be repeated';
//------------------------------------------------------------------------------
Expand All @@ -27,9 +28,22 @@ module.exports = {
ERROR_MESSAGE,

create(context) {
let importedEmberName;
let importedComputedName;

return {
ImportDeclaration(node) {
if (node.source.value === 'ember') {
importedEmberName = importedEmberName || getImportIdentifier(node, 'ember');
}
if (node.source.value === '@ember/object') {
importedComputedName =
importedComputedName || getImportIdentifier(node, '@ember/object', 'computed');
}
},

CallExpression(node) {
if (emberUtils.hasDuplicateDependentKeys(node)) {
if (emberUtils.hasDuplicateDependentKeys(node, importedEmberName, importedComputedName)) {
context.report({
node,
message: ERROR_MESSAGE,
Expand Down
16 changes: 15 additions & 1 deletion lib/rules/no-invalid-dependent-keys.js
View file
Open in desktop
Expand Up @@ -2,6 +2,7 @@

const types = require('../utils/types');
const ember = require('../utils/ember');
const { getImportIdentifier } = require('../utils/import');

//------------------------------------------------------------------------------
// General rule - Dependent keys used for computed properties have to be valid.
Expand Down Expand Up @@ -56,9 +57,22 @@ module.exports = {
ERROR_MESSAGE_INVALID_CHARACTER,

create(context) {
let importedEmberName;
let importedComputedName;

return {
ImportDeclaration(node) {
if (node.source.value === 'ember') {
importedEmberName = importedEmberName || getImportIdentifier(node, 'ember');
}
if (node.source.value === '@ember/object') {
importedComputedName =
importedComputedName || getImportIdentifier(node, '@ember/object', 'computed');
}
},

CallExpression(node) {
if (!ember.isComputedProp(node) || types.isMemberExpression(node.callee)) {
if (!ember.isComputedProp(node, importedEmberName, importedComputedName)) {
return;
}

Expand Down
21 changes: 13 additions & 8 deletions lib/rules/no-side-effects.js
View file
Open in desktop
@@ -1,11 +1,11 @@
'use strict';

const types = require('../utils/types');
const ember = require('../utils/ember');
const computedPropertyUtils = require('../utils/computed-properties');
const propertySetterUtils = require('../utils/property-setter');
const emberUtils = require('../utils/ember');
const Traverser = require('../utils/traverser');
const { getImportIdentifier } = require('../utils/import');

//------------------------------------------------------------------------------
// General rule - Don't introduce side-effects in computed properties
Expand Down Expand Up @@ -68,37 +68,42 @@ module.exports = {
ERROR_MESSAGE,

create(context) {
let emberImportAliasName;
let importedEmberName;
let importedComputedName;

const report = function (node) {
context.report(node, ERROR_MESSAGE);
};

return {
ImportDeclaration(node) {
if (!emberImportAliasName) {
emberImportAliasName = ember.getEmberImportAliasName(node);
if (node.source.value === 'ember') {
importedEmberName = importedEmberName || getImportIdentifier(node, 'ember');
}
if (node.source.value === '@ember/object') {
importedComputedName =
importedComputedName || getImportIdentifier(node, '@ember/object', 'computed');
}
},

CallExpression(node) {
if (!emberUtils.isComputedProp(node)) {
if (!emberUtils.isComputedProp(node, importedEmberName, importedComputedName)) {
return;
}

const computedPropertyBody = computedPropertyUtils.getComputedPropertyFunctionBody(node);

findSideEffects(computedPropertyBody, emberImportAliasName).forEach(report);
findSideEffects(computedPropertyBody, importedEmberName).forEach(report);
},

Identifier(node) {
if (!emberUtils.isComputedProp(node)) {
if (!emberUtils.isComputedProp(node, importedEmberName, importedComputedName)) {
return;
}

const computedPropertyBody = computedPropertyUtils.getComputedPropertyFunctionBody(node);

findSideEffects(computedPropertyBody, emberImportAliasName).forEach(report);
findSideEffects(computedPropertyBody, importedEmberName).forEach(report);
},
};
},
Expand Down
16 changes: 15 additions & 1 deletion lib/rules/no-volatile-computed-properties.js
View file
Open in desktop
Expand Up @@ -2,6 +2,7 @@

const types = require('../utils/types');
const emberUtils = require('../utils/ember');
const { getImportIdentifier } = require('../utils/import');

const ERROR_MESSAGE = 'Do not use volatile computed properties';

Expand All @@ -21,12 +22,25 @@ module.exports = {
},

create(context) {
let importedEmberName;
let importedComputedName;

return {
ImportDeclaration(node) {
if (node.source.value === 'ember') {
importedEmberName = importedEmberName || getImportIdentifier(node, 'ember');
}
if (node.source.value === '@ember/object') {
importedComputedName =
importedComputedName || getImportIdentifier(node, '@ember/object', 'computed');
}
},

CallExpression(node) {
if (
types.isMemberExpression(node.callee) &&
types.isCallExpression(node.callee.object) &&
emberUtils.isComputedProp(node.callee.object) &&
emberUtils.isComputedProp(node.callee.object, importedEmberName, importedComputedName) &&
types.isIdentifier(node.callee.property) &&
node.callee.property.name === 'volatile'
) {
Expand Down
16 changes: 15 additions & 1 deletion lib/rules/require-computed-macros.js
View file
Open in desktop
Expand Up @@ -5,6 +5,7 @@ const emberUtils = require('../utils/ember');
const propertyGetterUtils = require('../utils/property-getter');
const assert = require('assert');
const scopeReferencesThis = require('../utils/scope-references-this');
const { getImportIdentifier } = require('../utils/import');

//------------------------------------------------------------------------------
// Rule Definition
Expand Down Expand Up @@ -227,10 +228,23 @@ module.exports = {
});
}

let importedEmberName;
let importedComputedName;

return {
ImportDeclaration(node) {
if (node.source.value === 'ember') {
importedEmberName = importedEmberName || getImportIdentifier(node, 'ember');
}
if (node.source.value === '@ember/object') {
importedComputedName =
importedComputedName || getImportIdentifier(node, '@ember/object', 'computed');
}
},

// eslint-disable-next-line complexity
CallExpression(node) {
if (!emberUtils.isComputedProp(node)) {
if (!emberUtils.isComputedProp(node, importedEmberName, importedComputedName)) {
return;
}

Expand Down
17 changes: 2 additions & 15 deletions lib/rules/require-computed-property-dependencies.js
View file
Open in desktop
Expand Up @@ -47,19 +47,6 @@ function isMemberExpression(node, objectName, propertyName) {
);
}

/**
* @param {ASTNode} node
* @param {string} importedEmberName
* @param {string} importedComputedName
* @returns {boolean}
*/
function isEmberComputed(node, importedEmberName, importedComputedName) {
return (
isIdentifier(node, importedComputedName) ||
isMemberExpression(node, importedEmberName, 'computed')
);
}

/**
* Checks if a node looks like: 'part1' + 'part2'
*
Expand Down Expand Up @@ -481,7 +468,7 @@ module.exports = {
},

Identifier(node) {
if (isEmberComputed(node, importedEmberName, importedComputedName)) {
if (emberUtils.isComputedProp(node, importedEmberName, importedComputedName)) {
checkComputedDependencies(node, [], {
importedEmberName,
importedGetName,
Expand All @@ -492,7 +479,7 @@ module.exports = {
},

CallExpression(node) {
if (isEmberComputed(node.callee, importedEmberName, importedComputedName)) {
if (emberUtils.isComputedProp(node, importedEmberName, importedComputedName)) {
checkComputedDependencies(node, node.arguments, {
importedEmberName,
importedGetName,
Expand Down
26 changes: 24 additions & 2 deletions lib/rules/require-return-from-computed.js
View file
Open in desktop
@@ -1,6 +1,7 @@
'use strict';

const ember = require('../utils/ember');
const { getImportIdentifier } = require('../utils/import');

//------------------------------------------------------------------------------
// General rule - Always return a value from computed properties
Expand Down Expand Up @@ -46,12 +47,30 @@ module.exports = {
}
}

let importedEmberName;
let importedComputedName;

return {
ImportDeclaration(node) {
if (node.source.value === 'ember') {
importedEmberName = importedEmberName || getImportIdentifier(node, 'ember');
}
if (node.source.value === '@ember/object') {
importedComputedName =
importedComputedName || getImportIdentifier(node, '@ember/object', 'computed');
}
},

onCodePathStart(codePath) {
funcInfo = {
upper: funcInfo,
codePath,
shouldCheck: context.getAncestors().findIndex(ember.isComputedProp) > -1,
shouldCheck:
context
.getAncestors()
.findIndex((node) =>
ember.isComputedProp(node, importedEmberName, importedComputedName)
) > -1,
};
},

Expand All @@ -60,7 +79,10 @@ module.exports = {
},

'FunctionExpression:exit'(node) {
if (ember.isComputedProp(node.parent) || ember.isComputedProp(node.parent.parent.parent)) {
if (
ember.isComputedProp(node.parent, importedEmberName, importedComputedName) ||
ember.isComputedProp(node.parent.parent.parent, importedEmberName, importedComputedName)
) {
checkLastSegment(node);
}
},
Expand Down