Refactor null checks for new-module-imports and use-ember-data-rfc-395-imports rules

[dcyriller]

Refactor #460 and #461 to check against node types.

Updated fix for #459.

[bmish]

Thanks! Checking node types is much better than adding arbitrary null checks.

[bmish]

I tested this out on my codebase and found that it causes it at least one new bug that 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 (+ rebased). 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, this one 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 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. Because these functions implicitely rely on a visitor scope.

If, with this explanation, you have some more ideas @bmish, let me know!

[dcyriller]

I've just fixed the rebase conflicts.