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.

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

Add no-shallow-imports rule #2408

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange
- [`no-named-default`, `no-default-export`, `prefer-default-export`, `no-named-export`, `export`, `named`, `namespace`, `no-unused-modules`]: support arbitrary module namespace names ([#2358], thanks [@sosukesuzuki])
- [`no-dynamic-require`]: support dynamic import with espree ([#2371], thanks [@sosukesuzuki])
- [`no-relative-packages`]: add fixer ([#2381], thanks [@forivall])
- Add [`no-shallow-imports`] rule
Copy link
Member

Choose a reason for hiding this comment

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

this line needs to be moved up to the "unreleased" section

Copy link
Author

Choose a reason for hiding this comment

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

Good point! :)


### Fixed
- [`default`]: `typescript-eslint-parser`: avoid a crash on exporting as namespace (thanks [@ljharb])
Expand Down
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ This plugin intends to support linting of ES2015+ (ES6+) import/export syntax, a
* Prevent unnecessary path segments in import and require statements ([`no-useless-path-segments`])
* Forbid importing modules from parent directories ([`no-relative-parent-imports`])
* Prevent importing packages through relative paths ([`no-relative-packages`])
* Prevent the use of shallow (barrel) imports ([`no-shallow-imports`])

[`no-unresolved`]: ./docs/rules/no-unresolved.md
[`named`]: ./docs/rules/named.md
Expand All @@ -44,6 +45,7 @@ This plugin intends to support linting of ES2015+ (ES6+) import/export syntax, a
[`no-useless-path-segments`]: ./docs/rules/no-useless-path-segments.md
[`no-relative-parent-imports`]: ./docs/rules/no-relative-parent-imports.md
[`no-relative-packages`]: ./docs/rules/no-relative-packages.md
[`no-shallow-imports`]: ./docs/rules/no-shallow-imports.md

### Helpful warnings

Expand Down
64 changes: 64 additions & 0 deletions docs/rules/no-shallow-imports.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
# import/no-shallow-imports

Prevent the use of shallow (barrel) imports.


## Options

There's one option available:
* `allow`: an array of minimatch/glob patterns that match paths which allow shallow importing (exempt from the rule)

### Example rule (barebones)

```js
"import/no-shallow-imports": "error"
```

### Example rule (with option)

```js
"import/no-shallow-imports": ["error", {
"allow": ["**/index.test.*"]
}]
```

## Examples

Given the following structure, along with the example rule (with option) above....

```
my-project
├── dir1
│ └── example1.js
│ └── index.js
└── dir2
│ └── example2.js
│ └── index.js
└── index.js
└── index.test.js
```


### Examples of **incorrect** code for this rule:

```js
// dir1/example1.js

import example2 from '..'; // imports from /index.js
import example2 from '../dir2'; // imports from /dir2/index.js
import example2 from '../dir2/index'; // same as above but explicit
```
Comment on lines +47 to +50
Copy link
Member

Choose a reason for hiding this comment

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

Why are these incorrect? What if dir2/index has a single default export?


### Examples of **correct** code for this rule:

```js
// dir1/example1.js

import example2 from '../dir2/example2'; // deep import :D
```

```js
// index.test.js

import * as index from './index'; // allowed
Copy link
Member

Choose a reason for hiding this comment

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

import * is a substandard pattern that should never be encouraged.

Choose a reason for hiding this comment

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

Should import * be disallowed by this rule?

Copy link
Member

Choose a reason for hiding this comment

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

Not necessarily, but the docs shouldn't suggest it.

Choose a reason for hiding this comment

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

I agree it shouldn't suggest it, but I think it is important for the docs to show that the pattern is still allowed with this rule - as users may assume that it is disallowed or vice versa. Maybe the docs should show it as correct but discouraged / not recommended?

Suggested change
import * as index from './index'; // allowed
import * as index from './index'; // allowed but discouraged (to disallow this, use the `import/no-namespace` rule)

Copy link
Member

Choose a reason for hiding this comment

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

Sure, that's a reasonable compromise.

```
1 change: 1 addition & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export const rules = {
'group-exports': require('./rules/group-exports'),
'no-relative-packages': require('./rules/no-relative-packages'),
'no-relative-parent-imports': require('./rules/no-relative-parent-imports'),
'no-shallow-imports': require('./rules/no-shallow-imports'),

'no-self-import': require('./rules/no-self-import'),
'no-cycle': require('./rules/no-cycle'),
Expand Down
104 changes: 104 additions & 0 deletions src/rules/no-shallow-imports.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
import fs from 'fs';
import minimatch from 'minimatch';
import docsUrl from '../docsUrl';

module.exports = {
meta: {
type: 'problem',
docs: {
description: 'Prevent the use of shallow imports',
recommended: true,
url: docsUrl('no-shallow-imports'),
},
schema: [
{
oneOf: [
{
type: 'object',
properties: {
allow: {
type: 'array',
items: {
type: 'string',
},
},
},
additionalProperties: false,
},
],
},
],
},
create: context => {
return {
ImportDeclaration: node => maybeReportShallowImport(node, context),
};
},
};

const maybeReportShallowImport = (node, context) => {
const filenamePath = context.getFilename();

const options = context.options[0] || {};
const allowRegexps = (options.allow || []).map(p => minimatch.makeRe(p));

const doesFileMatchAllowRegexps = allowRegexps.some(regex => regex.test(filenamePath));

// If the file does not match the regexps and is a shallow import, then report it
if (!doesFileMatchAllowRegexps && isShallowImport(node, context)) {
return context.report({
node,
message: 'Please deep import!',
});
}
};

const isCalledIndex = filename => filename.slice(0, 5) === 'index';

Choose a reason for hiding this comment

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

Suggested change
const isCalledIndex = filename => filename.slice(0, 5) === 'index';
const isCalledIndex = filename => filename.split('.')[0] === 'index';

Think this might be slightly better because otherwise:

isCalledIndex('index-lorem-ipsum.tsx') 

will be true where as it should be false.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const isCalledIndex = filename => filename.slice(0, 5) === 'index';
const isCalledIndex = filename => path.basename(filename, path.extname(filename)) === 'index';

Choose a reason for hiding this comment

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

@ljharb path.basename(filename, path.extname(filename)) === 'index' would evaluate to false if the filename is index.test.js for example

filename.split('.')[0] === 'index' might be safer - what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, that sounds good


const isShallowImport = (node, context) => {
const filenamePath = context.getFilename();
const sourcePath = node.source.value;
const segments = sourcePath.split('/');

Choose a reason for hiding this comment

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

Suggested change
const segments = sourcePath.split('/');

const lastSegment = segments[segments.length - 1];

Choose a reason for hiding this comment

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

Suggested change
const lastSegment = segments[segments.length - 1];
const lastSegment = path.basename(filenamePath, path.extname(filenamePath));


if (sourcePath[0] !== '.') {
return false; // not a relative path, ie. it does not start with '.' or '..'
}

// Source ends in '..' (eg. '..', '../..', etc.)
if (lastSegment === '..') return true;

// Source ends in 'index(\..*)?' (eg. 'index', 'index.js', 'index.etc')
if (isCalledIndex(lastSegment)) return true;

// Source is a directory
if (isDirectory(filenamePath, sourcePath)) return true;

return false;
};

const isDirectory = (filename, source) => {
const filenameSegments = filename.split('/');
const sourceSegments = source.split('/');

if (sourceSegments[0] === '.') {
sourceSegments.shift();
}

while (sourceSegments[0] === '..') {
filenameSegments.pop();
sourceSegments.shift();
}

// Swap out the last element in filenameSegments with the remaining sourceSegments
filenameSegments.splice(filenameSegments.length - 1, 1, ...sourceSegments);

const absoluteSource = filenameSegments.join('/');

try {
const stat = fs.statSync(absoluteSource);
return stat.isDirectory();
} catch (e) {
return false;
}
};
47 changes: 47 additions & 0 deletions tests/src/rules/no-shallow-imports.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import { test, testFilePath } from '../utils';
const rule = require('../../../src/rules/no-shallow-imports');
const { RuleTester } = require('eslint');

const ruleTester = new RuleTester();
const errors = [{ message: 'Please deep import!' }];
const options = [{ 'allow': ['**/index.test.js'] }];
const filename = testFilePath('./index.test.js');

ruleTester.run('no-shallow-imports', rule, {
valid: [
test({
code: 'import { module } from "package"',
}),
test({
code: 'import { module } from "@scope/package"',
}),
test({
code: 'import * as index from "../index"',
options,
filename,
}),
],

invalid: [
test({
code: 'import { barrel } from ".."',
errors,
}),
test({
code: 'import { barrel } from "../.."',
errors,
}),
// test({
// code: `import { barrel } from '../../dir';`,
// errors,
// }),
test({
code: 'import { barrel } from "../dir/index"',
errors,
}),
test({
code: 'import { barrel } from "./dir/index.js"',
errors,
}),
],
});