Fix #570: No newline after inline require (#579)

* Breaking - newline-after-import: Do not require empty line after inline require (#570) * Simplify newline-after-import implementation
import-js · Sep 26, 2016 · 29e1874 · 29e1874
1 parent 1d5b14f
commit 29e1874
Show file tree

Hide file tree

Showing 4 changed files with 83 additions and 55 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel
 ### Breaking
 - [`import/extensions` setting] defaults to `['.js']`. ([#306])
 - [`import/ignore` setting] defaults to nothing, and ambiguous modules are ignored natively. This means importing from CommonJS modules will no longer be reported by [`default`], [`named`], or [`namespace`], regardless of `import/ignore`. ([#270])
+- [`newline-after-import`]: Removed need for an empty line after an inline `require` call ([#570])
 
 ### Changed
 - `imports-first` is renamed to [`first`]. `imports-first` alias will continue to
@@ -381,6 +382,7 @@ for info on changes for earlier releases.
 [#157]: https://github.com/benmosher/eslint-plugin-import/pull/157
 [#314]: https://github.com/benmosher/eslint-plugin-import/pull/314
 
+[#570]: https://github.com/benmosher/eslint-plugin-import/issues/570
 [#567]: https://github.com/benmosher/eslint-plugin-import/issues/567
 [#566]: https://github.com/benmosher/eslint-plugin-import/issues/566
 [#545]: https://github.com/benmosher/eslint-plugin-import/issues/545

diff --git a/docs/rules/newline-after-import.md b/docs/rules/newline-after-import.md
@@ -1,11 +1,9 @@
 # newline-after-import
 
-Reports if there's no new line after last import/require in group.
+Enforces having an empty line after the last top-level import statement or require call.
 
 ## Rule Details
 
-**NOTE**: In each of those examples you can replace `import` call with `require`.
-
 Valid:
 
 ```js
@@ -21,6 +19,13 @@ import { bar }  from 'bar-lib'
 const FOO = 'BAR'
 ```
 
+```js
+const FOO = require('./foo')
+const BAR = require('./bar')
+
+const BAZ = 1
+```
+
 ...whereas here imports will be reported:
 
 ```js
@@ -35,6 +40,12 @@ const FOO = 'BAR'
 import { bar }  from 'bar-lib'
 ```
 
+```js
+const FOO = require('./foo')
+const BAZ = 1
+const BAR = require('./bar')
+```
+
 ## When Not To Use It
 
 If you like to visually group module imports with its usage, you don't want to use this rule.
diff --git a/src/rules/newline-after-import.js b/src/rules/newline-after-import.js
@@ -45,10 +45,9 @@ module.exports = {
   meta: {
     docs: {},
   },
-
   create: function (context) {
-    const scopes = []
-    let scopeIndex = 0
+    let level = 0
+    const requireCalls = []
 
     function checkForNewLine(node, nextNode, type) {
       if (getLineDifference(node, nextNode) < 2) {
@@ -68,6 +67,13 @@ module.exports = {
       }
     }
 
+    function incrementLevel() {
+      level++
+    }
+    function decrementLevel() {
+      level--
+    }
+
     return {
       ImportDeclaration: function (node) {
         const { parent } = node
@@ -78,55 +84,45 @@ module.exports = {
           checkForNewLine(node, nextNode, 'import')
         }
       },
-      Program: function () {
-        scopes.push({ scope: context.getScope(), requireCalls: [] })
-      },
       CallExpression: function(node) {
-        const scope = context.getScope()
-        if (isStaticRequire(node)) {
-          const currentScope = scopes[scopeIndex]
-
-          if (scope === currentScope.scope) {
-            currentScope.requireCalls.push(node)
-          } else {
-            scopes.push({ scope, requireCalls: [ node ] })
-            scopeIndex += 1
-          }
+        if (isStaticRequire(node) && level === 0) {
+          requireCalls.push(node)
         }
       },
       'Program:exit': function () {
         log('exit processing for', context.getFilename())
-        scopes.forEach(function ({ scope, requireCalls }) {
-          const scopeBody = getScopeBody(scope)
+        const scopeBody = getScopeBody(context.getScope())
+        log('got scope:', scopeBody)
 
-          // skip non-array scopes (i.e. arrow function expressions)
-          if (!scopeBody || !(scopeBody instanceof Array)) {
-            log('invalid scope:', scopeBody)
-            return
-          }
-
-          log('got scope:', scopeBody)
-
-          requireCalls.forEach(function (node, index) {
-            const nodePosition = findNodeIndexInScopeBody(scopeBody, node)
-            log('node position in scope:', nodePosition)
+        requireCalls.forEach(function (node, index) {
+          const nodePosition = findNodeIndexInScopeBody(scopeBody, node)
+          log('node position in scope:', nodePosition)
 
-            const statementWithRequireCall = scopeBody[nodePosition]
-            const nextStatement = scopeBody[nodePosition + 1]
-            const nextRequireCall = requireCalls[index + 1]
+          const statementWithRequireCall = scopeBody[nodePosition]
+          const nextStatement = scopeBody[nodePosition + 1]
+          const nextRequireCall = requireCalls[index + 1]
 
-            if (nextRequireCall && containsNodeOrEqual(statementWithRequireCall, nextRequireCall)) {
-              return
-            }
+          if (nextRequireCall && containsNodeOrEqual(statementWithRequireCall, nextRequireCall)) {
+            return
+          }
 
-            if (nextStatement &&
-               (!nextRequireCall || !containsNodeOrEqual(nextStatement, nextRequireCall))) {
+          if (nextStatement &&
+             (!nextRequireCall || !containsNodeOrEqual(nextStatement, nextRequireCall))) {
 
-              checkForNewLine(statementWithRequireCall, nextStatement, 'require')
-            }
-          })
+            checkForNewLine(statementWithRequireCall, nextStatement, 'require')
+          }
         })
       },
+      FunctionDeclaration: incrementLevel,
+      FunctionExpression: incrementLevel,
+      ArrowFunctionExpression: incrementLevel,
+      BlockStatement: incrementLevel,
+      ObjectExpression: incrementLevel,
+      'FunctionDeclaration:exit': decrementLevel,
+      'FunctionExpression:exit': decrementLevel,
+      'ArrowFunctionExpression:exit': decrementLevel,
+      'BlockStatement:exit': decrementLevel,
+      'ObjectExpression:exit': decrementLevel,
     }
   },
 }
diff --git a/tests/src/rules/newline-after-import.js b/tests/src/rules/newline-after-import.js
@@ -1,7 +1,7 @@
 import { RuleTester } from 'eslint'
 
-const IMPORT_ERROR_MESSAGE = 'Expected empty line after import statement not followed by another import.';
-const REQUIRE_ERROR_MESSAGE = 'Expected empty line after require statement not followed by another require.';
+const IMPORT_ERROR_MESSAGE = 'Expected empty line after import statement not followed by another import.'
+const REQUIRE_ERROR_MESSAGE = 'Expected empty line after require statement not followed by another require.'
 
 const ruleTester = new RuleTester()
 
@@ -21,7 +21,6 @@ ruleTester.run('newline-after-import', require('rules/newline-after-import'), {
       parserOptions: { ecmaVersion: 6 } ,
     },
     "function x(){ require('baz'); }",
-
     "a(require('b'), require('c'), require('d'));",
     `function foo() {
       switch (renderData.modalViewKey) {
@@ -100,7 +99,35 @@ ruleTester.run('newline-after-import', require('rules/newline-after-import'), {
     {
       code: "var foo = require('foo-module');\n\nvar a = 123;\n\nvar bar = require('bar-lib');",
       parserOptions: { sourceType: 'module' }
-    }
+    },
+    {
+      code: `
+        function foo() {
+          var foo = require('foo');
+          foo();
+        }
+      `,
+      parserOptions: { sourceType: 'module' },
+    },
+    {
+      code: `
+        if (true) {
+          var foo = require('foo');
+          foo();
+        }
+      `,
+      parserOptions: { sourceType: 'module' },
+    },
+    {
+      code: `
+        function a() {
+          var assign = Object.assign || require('object-assign');
+          var foo = require('foo');
+          var bar = 42;
+        }
+      `,
+      parserOptions: { sourceType: 'module' },
+    },
   ],
 
   invalid: [
@@ -183,14 +210,6 @@ ruleTester.run('newline-after-import', require('rules/newline-after-import'), {
         message: REQUIRE_ERROR_MESSAGE,
       } ]
     },
-    {
-      code: "function a() {\nvar assign = Object.assign || require('object-assign');\nvar foo = require('foo');\nvar bar = 42; }",
-      errors: [ {
-        line: 3,
-        column: 1,
-        message: REQUIRE_ERROR_MESSAGE,
-      } ]
-    },
     {
       code: "require('a');\nfoo(require('b'), require('c'), require('d'));\nrequire('d');\nvar foo = 'bar';",
       errors: [