Repository: ember-cli/eslint-plugin-ember
Pull request #462 (2019-08-09 11:47:56)

Refactor null checks for `new-module-imports` and `use-ember-data-rfc-395-imports` rules
Refactor https://github.com/ember-cli/eslint-plugin-ember/pull/460 and https://github.com/ember-cli/eslint-plugin-ember/pull/461 to check against node types.

Updated fix for #459.

bmish: I tested this out on my codebase and found that it causes it at least one bug must be fixed before merging:

```
new-module-imports › valid › function myFunction(list) { for (const item in list) { } }

    TypeError: Cannot read property 'type' of null
    Occurred while linting <input>:1

      155 | 
      156 | function isDestructuring(node) {
    > 157 |   return node.type === 'VariableDeclarator' && node.init.type === 'Identifier';
          |                                                          ^
      158 | }
      159 | 
      160 | function isIdentifierImportedFrom(node, module) {

      at type (lib/utils/new-module.js:157:58)
      at isDestructuring (lib/rules/new-module-imports.js:61:14)
```

dcyriller: Looks like it’s not ready! I will reopen when I’ll be able to move it forward.
dcyriller: @bmish I've applied your suggestion. I've manually tested it against an app and and addon. It should be good now!
bmish: @dcyriller thanks! Could you add some test cases in the following files so that we can be confident all of this logic works correctly?

* tests/lib/rules/new-module-imports.js
* tests/lib/rules/use-ember-data-rfc-395-imports.js
* tests/lib/utils/new-module-test.js (new test file)
dcyriller: While adding tests for these utils, I’ve learnt something worth sharing.

A visitor’s node (as in this snippet:)
```
module.exports = {
    meta: { ... },
    create(context) {
        return {
            ReturnStatement(node) {
            },
        }
    }
};
```
is not a bare node. I mean: it is not a node as parsed by espree or babel-eslint. A node parsed by espree doesn't have a `parent` key. Yet, it is decorated with its `parent`. Which means the visitor's node has actually been post processed (during the visitor instanciation?).

This observation has a consequence: a lot of utils are implicitely tied to the visitor. This makes them pretty hard to test on their own. I guess this is why an util like [getParent](https://github.com/ember-cli/eslint-plugin-ember/blob/v7.1.0/lib/utils/utils.js#L206) is not tested. And this is why I can't add most of the test files you're asking.

I mean these utils are covered by the rules tests which are higher level. But we can't test them at a lower level as bare function.

If, with this explanation, you have some more ideas @bmish, let me know!

Patch:
diff --git a/lib/rules/new-module-imports.js b/lib/rules/new-module-imports.js
index 4a953642..08daa6b9 100644
--- a/lib/rules/new-module-imports.js
+++ b/lib/rules/new-module-imports.js
@@ -1,7 +1,12 @@
 'use strict';
 
 const MAPPING = require('ember-rfc176-data');
-const { buildMessage, getFullNames, isInitImportedFrom } = require('../utils/new-module');
+const {
+  buildMessage,
+  getFullNames,
+  isDestructuring,
+  isIdentifierImportedFrom,
+} = require('../utils/new-module');
 
 const GLOBALS = MAPPING.reduce((memo, exportDefinition) => {
   if (exportDefinition.deprecated) {
@@ -53,12 +58,7 @@ module.exports = {
 
     return {
       VariableDeclarator(node) {
-        if (!node.init) {
-          return;
-        }
-
-        // Filter out non-Ember variable declarations
-        if (node.init.name !== 'Ember' && !isInitImportedFrom(node, 'ember')) {
+        if (!isDestructuring(node) || !isIdentifierImportedFrom(node, 'ember')) {
           return;
         }
 
diff --git a/lib/rules/use-ember-data-rfc-395-imports.js b/lib/rules/use-ember-data-rfc-395-imports.js
index 63ccf134..1d1296b4 100644
--- a/lib/rules/use-ember-data-rfc-395-imports.js
+++ b/lib/rules/use-ember-data-rfc-395-imports.js
@@ -1,7 +1,13 @@
 'use strict';
 
 const MAPPINGS = require('@ember-data/rfc395-data');
-const { buildFix, buildMessage, getFullNames, isInitImportedFrom } = require('../utils/new-module');
+const {
+  buildFix,
+  buildMessage,
+  getFullNames,
+  isDestructuring,
+  isIdentifierImportedFrom,
+} = require('../utils/new-module');
 
 /**
  * This function returns an object like this:
@@ -109,12 +115,8 @@ module.exports = {
        * and against `const { Model } = ED` (only) if ED is imported from ember-data
        */
       VariableDeclarator(node) {
-        if (!node.init) {
-          return;
-        }
-
-        // Filter out non-DS variable declarations
-        if (node.init.name !== 'DS' && !isInitImportedFrom(node, 'ember-data')) {
+        // Filter out non-ember-data variable declarations
+        if (!isDestructuring(node) || !isIdentifierImportedFrom(node, 'ember-data')) {
           return;
         }
 
diff --git a/lib/utils/new-module.js b/lib/utils/new-module.js
index 5d2a73b1..bef47468 100644
--- a/lib/utils/new-module.js
+++ b/lib/utils/new-module.js
@@ -1,5 +1,7 @@
 'use strict';
 
+const { isIdentifier, isObjectPattern, isVariableDeclarator } = require('./types');
+
 const EMBER_NAMESPACES = ['inject.controller', 'inject.service'];
 
 // Both the Controller module and Service module each make available the
@@ -153,11 +155,23 @@ function getFullNames(prefix, node) {
   return fullNames;
 }
 
-function isInitImportedFrom(node, module) {
+function isDestructuring(node) {
+  return (
+    isVariableDeclarator(node) &&
+    node.init &&
+    isIdentifier(node.init) &&
+    node.id &&
+    isObjectPattern(node.id)
+  );
+}
+
+function isIdentifierImportedFrom(node, module) {
   const { name } = node.init;
   const pp = node.parent.parent;
 
-  if (!pp || !pp.body || !pp.body.some) {
+  // `import` may only appear at the top level, ie. in a `Program` node
+  // also, a `Program` node will always have a `body` attribute
+  if (!pp || pp.type !== 'Program') {
     return false;
   }
 
@@ -180,5 +194,6 @@ module.exports = {
   buildFix,
   buildMessage,
   getFullNames,
-  isInitImportedFrom,
+  isDestructuring,
+  isIdentifierImportedFrom,
 };


Test patch:
diff --git a/tests/lib/utils/new-module-test.js b/tests/lib/utils/new-module-test.js
new file mode 100644
index 00000000..00955951
--- /dev/null
+++ b/tests/lib/utils/new-module-test.js
@@ -0,0 +1,35 @@
+'use strict';
+
+const babelEslint = require('babel-eslint');
+const { buildFix, isDestructuring } = require('../../../lib/utils/new-module');
+
+const modulesData = {
+  'ember-data/model': {
+    default: ['@ember-data/model'],
+  },
+};
+
+describe('isDestructuring', () => {
+  it('should check a destructuring import', () => {
+    const node = babelEslint.parse(`const { destructured } = someVar;`).body[0].declarations[0];
+    expect(isDestructuring(node)).toBeTruthy();
+  });
+
+  it('should check a non-destructuring import', () => {
+    const node = babelEslint.parse(`const destructured = someVar;`).body[0].declarations[0];
+    expect(isDestructuring(node)).toBeFalsy();
+  });
+
+  it('should check a ForInStatement', () => {
+    const node = babelEslint.parse(`for (const item in list) {};`).body[0].left.declarations[0];
+    expect(isDestructuring(node)).toBeFalsy();
+  });
+});
+
+describe('buildFix', () => {
+  it('returns a function', () => {
+    const node = babelEslint.parse(`import Model from "ember-data/model"`).body[0];
+    const fix = buildFix(node, modulesData);
+    expect(typeof fix === 'function').toBeTruthy();
+  });
+});