Adds `no-ancestor-directory-import` rule #149

cartogram · 2018-08-28T09:20:08Z

Closes #138

In addition to the code, feedback on the description and name of this rule would be really great. I found it to be a hard one to describe in words.

kaelig · 2018-08-28T20:18:17Z

docs/rules/prefer-explicit-local-imports.md

+
+## Rule Details
+
+This rule aims to...


Is this sentence incomplete?

@kaelig updated

lemonmade · 2018-08-29T19:13:49Z

docs/rules/prefer-explicit-local-imports.md

@@ -0,0 +1,27 @@
+# Prefer relative local imports extend to the file from where they are importing without relying on an index file. (prefer-explicit-local-imports)


I think I might describe this more as "no ancestor directory import".

Yeah, all makes sense. I like that name.

lemonmade · 2018-08-29T19:14:50Z

lib/rules/prefer-explicit-local-imports.js

+  },
+};
+
+function isVagueImport(str) {


It isn't really vague, it's more about the fact that it is a direct ancestor.

lemonmade · 2018-08-29T19:16:36Z

lib/rules/prefer-explicit-local-imports.js

+  create(context) {
+    return {
+      ImportDeclaration(node) {
+        const pathParts = node.source.value.split('/');


Use path.sep

lemonmade · 2018-08-29T19:17:19Z

lib/rules/prefer-explicit-local-imports.js

+    return str === '' || str === '.' || str === '..';
+  }
+
+  return basename(str, extname(str)) === 'index';


I think you are better off resolving the path and then checking that either it is a directory and an ancestor of the current path, or it is an index file of such a directory. This prevents potential issues with this method, as this incorrectly warns on some weirder import paths that we'd still consider technically valid

some weirder import paths that we'd still consider technically valid

What would some of these cases be? It would be good to add them to the tests

ismail-syed · 2018-08-29T19:40:55Z

tests/lib/rules/prefer-explicit-local-imports.js

+    },
+  ],
+
+  invalid: [


Maybe add a few valid and invalid tests for importing non-default exports
eg. import {Foo, Bar} from '../'

ismail-syed · 2018-08-29T19:44:03Z

CHANGELOG.md

@@ -14,6 +14,7 @@
 ### Added
 * `shopify/prefer-singular-enums` ([#132](https://github.com/Shopify/eslint-plugin-shopify/pull/132))
 * `shopify/react-no-multiple-render-methods` ([#134](https://github.com/Shopify/eslint-plugin-shopify/pull/134))
+* `shopify/prefer-explicit-local-imports` ([#149](https://github.com/Shopify/eslint-plugin-shopify/pull/149))


shopify/prefer-namespaced-imports might be a better name - not entirely sure.

ismail-syed

👍 Nice one. Nothing major from my end, just a couple comments.

cartogram · 2018-09-30T22:30:19Z

@lemonmade ready for another look. Thanks!

cc/ @BPScott since you ran into this recently.

BPScott

Nice. A few more test cases around components reaching into sibling components, and subcomponents would be handy. If I've understood the purpose of this PR then the following should all pass:

{
    code: "import Bar from '../Bar'",
    filename: fixtureFile('basic-app/app/components/Foo/Foo.js'),
},
{
    code: "import Bar from '..'",
    filename: fixtureFile('basic-app/app/components/Foo/Foo.js'),
},
{
    code: "import Bar from '../Bar'",
    filename: fixtureFile('basic-app/app/components/Foo/components/Baz/Baz.js'),
},
{
    code: "import Qux from '..'",
    filename: fixtureFile('basic-app/app/components/Foo/components/Baz/Baz.js'),
},
{
    code: "import Qux from '../Qux'",
    filename: fixtureFile('basic-app/app/components/Foo/components/Baz/Baz.js'),
},
{
    code: "import Bar from '../../../Bar'",
    filename: fixtureFile('basic-app/app/components/Foo/components/Baz/Baz.js'),
},
{
    code: "import Bar from '../../..'",
    filename: fixtureFile('basic-app/app/components/Foo/components/Baz/Baz.js'),
},

BPScott · 2018-10-01T18:33:24Z

tests/lib/rules/no-ancestor-directory-import.js

+const {fixtureFile} = require('../../utilities');
+const rule = require('../../../lib/rules/no-ancestor-directory-import');
+
+const ruleTester = new RuleTester();


I think you can do

const parserOptions = new RuleTester({parserOptions: { ecmaVersion: 6, sourceType: 'module', }})

to avoid having to pass parserOptions into every valid/invalid block below. (see https://eslint.org/docs/developer-guide/nodejs-api#ruletester)

TIL! Thanks @BPScott

lemonmade · 2018-10-02T00:44:09Z

docs/rules/prefer-explicit-local-imports.md

@@ -0,0 +1,27 @@
+# Prefer imports from within a directory extend to the file from where they are importing without relying on an index file. (no-ancestor-directory-import)


Prefer that

lemonmade · 2018-10-02T00:44:26Z

lib/rules/no-ancestor-directory-import.js

+  meta: {
+    docs: {
+      description:
+        'Prefer imports from within a directory extend to the file from where they are importing without relying on an index file.',


lemonmade · 2018-10-02T00:44:34Z

lib/rules/no-ancestor-directory-import.js

+    docs: {
+      description:
+        'Prefer imports from within a directory extend to the file from where they are importing without relying on an index file.',
+      category: 'Best Practises',


Best practices

lemonmade · 2018-10-02T00:45:30Z

lib/rules/no-ancestor-directory-import.js

+    fixable: null,
+  },
+  create(context) {
+    function isAncestorDirectoryImport(resolvedSource) {


This function doesn't seem to quite line up with what it checks

lemonmade · 2018-10-04T16:12:07Z

docs/rules/no-ancestor-directory-import.md

@@ -0,0 +1,27 @@
+# Prefer that imports from within a directory extend to the file from where they are importing without relying on an index file. (no-ancestor-directory-import)
+
+Imports inside the same directory should extend directly to the file from where they are importing without relying on an index file. This means the source of these imports should not end with a directory (`/`), but the path should terminate at an individual filename. This preserves the index file inside a directory as a mechanism for exposing pieces of the module to the outside application.


The last sentence should be augmented with , rather than as a way to export the parts that the module internally depends on. or something like that.

lemonmade · 2018-10-04T16:15:08Z

lib/rules/no-ancestor-directory-import.js

+          context.report({
+            node,
+            message:
+              'Relative local imports should extend to the file from where they are importing without relying on an index file.',


I don't know if this message is quite right; since we resolve the source, it might not even be a relative import (i.e., something in app/sections asking for index would fail this rule, as that likely resolves to app/index.js

lemonmade · 2018-10-04T16:15:51Z

lib/rules/no-ancestor-directory-import.js

+        resolvedSource,
+      );
+
+      const parts = relativeDifference


So this just strips out all the leading ./../../ stuff right?

lemonmade · 2018-10-04T16:16:36Z

tests/lib/rules/no-ancestor-directory-import.js

+      filename: fixtureFile('basic-app/app/components/Foo/Foo.js'),
+    },
+    {
+      code: "import Bar from '../../..'",


Might want to check that ../../../ works also (trailing slash)

cartogram force-pushed the prefer-explicit-relative-imports branch from 85bd95c to 1a6c03e Compare August 28, 2018 17:18

kaelig reviewed Aug 28, 2018

View reviewed changes

cartogram force-pushed the prefer-explicit-relative-imports branch from 1a6c03e to 008aefc Compare August 29, 2018 03:53

cartogram requested a review from GoodForOneFare August 29, 2018 04:54

cartogram force-pushed the prefer-explicit-relative-imports branch 3 times, most recently from 278b76f to e6d1fa9 Compare August 29, 2018 05:04

cartogram requested review from ismail-syed and lemonmade and removed request for GoodForOneFare August 29, 2018 18:28

lemonmade suggested changes Aug 29, 2018

View reviewed changes

ismail-syed reviewed Aug 29, 2018

View reviewed changes

ismail-syed approved these changes Aug 29, 2018

View reviewed changes

cartogram force-pushed the prefer-explicit-relative-imports branch 5 times, most recently from 8f22080 to 1723535 Compare September 30, 2018 21:57

cartogram changed the title ~~Adds prefer-explicit-local-imports rule~~ Adds no-ancestor-directory-import rule Sep 30, 2018

cartogram mentioned this pull request Sep 30, 2018

Adds eslint-plugin-eslint-comments #164

Merged

BPScott reviewed Oct 1, 2018

View reviewed changes

lemonmade approved these changes Oct 2, 2018

View reviewed changes

cartogram force-pushed the prefer-explicit-relative-imports branch 4 times, most recently from e417285 to e820e7a Compare October 2, 2018 02:29

lemonmade approved these changes Oct 4, 2018

View reviewed changes

lemonmade reviewed Oct 4, 2018

View reviewed changes

cartogram force-pushed the prefer-explicit-relative-imports branch 3 times, most recently from ecce01b to e257b2d Compare October 15, 2018 14:25

cartogram force-pushed the prefer-explicit-relative-imports branch 2 times, most recently from 9434d5b to 7b8a75a Compare October 27, 2018 20:04

no-ancestor-directory-import

414db1d

cartogram force-pushed the prefer-explicit-relative-imports branch from 7b8a75a to 414db1d Compare October 27, 2018 20:06

cartogram merged commit fede0a0 into master Oct 27, 2018

cartogram deleted the prefer-explicit-relative-imports branch October 27, 2018 20:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds `no-ancestor-directory-import` rule #149

Adds `no-ancestor-directory-import` rule #149

cartogram commented Aug 28, 2018 •

edited

Loading

kaelig Aug 28, 2018

cartogram Aug 29, 2018

lemonmade Aug 29, 2018

cartogram Sep 30, 2018

lemonmade Aug 29, 2018

lemonmade Aug 29, 2018

lemonmade Aug 29, 2018

ismail-syed Aug 29, 2018 •

edited

Loading

ismail-syed Aug 29, 2018

ismail-syed Aug 29, 2018

ismail-syed left a comment

cartogram commented Sep 30, 2018 •

edited

Loading

BPScott left a comment

BPScott Oct 1, 2018

cartogram Oct 2, 2018

lemonmade Oct 2, 2018

lemonmade Oct 2, 2018

lemonmade Oct 2, 2018

lemonmade Oct 2, 2018

lemonmade Oct 4, 2018

lemonmade Oct 4, 2018

lemonmade Oct 4, 2018

lemonmade Oct 4, 2018

		@@ -0,0 +1,27 @@
		# Prefer relative local imports extend to the file from where they are importing without relying on an index file. (prefer-explicit-local-imports)

		@@ -0,0 +1,27 @@
		# Prefer imports from within a directory extend to the file from where they are importing without relying on an index file. (no-ancestor-directory-import)

		@@ -0,0 +1,27 @@
		# Prefer that imports from within a directory extend to the file from where they are importing without relying on an index file. (no-ancestor-directory-import)

		Imports inside the same directory should extend directly to the file from where they are importing without relying on an index file. This means the source of these imports should not end with a directory (`/`), but the path should terminate at an individual filename. This preserves the index file inside a directory as a mechanism for exposing pieces of the module to the outside application.

Adds no-ancestor-directory-import rule #149

Adds no-ancestor-directory-import rule #149

Conversation

cartogram commented Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ismail-syed Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ismail-syed left a comment

Choose a reason for hiding this comment

cartogram commented Sep 30, 2018 • edited Loading

BPScott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adds `no-ancestor-directory-import` rule #149

Adds `no-ancestor-directory-import` rule #149

cartogram commented Aug 28, 2018 •

edited

Loading

ismail-syed Aug 29, 2018 •

edited

Loading

cartogram commented Sep 30, 2018 •

edited

Loading