-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
base: main
Are you sure you want to change the base?
Conversation
@ljharb Hey Jordan, just wondering if there's any feedback on this pull request. Cheers :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should work by looking at the consuming side - i think it should work by looking at the exporting side.
@@ -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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! :)
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 | ||
``` |
There was a problem hiding this comment.
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?
```js | ||
// index.test.js | ||
|
||
import * as index from './index'; // allowed |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
import * as index from './index'; // allowed | |
import * as index from './index'; // allowed but discouraged (to disallow this, use the `import/no-namespace` rule) |
There was a problem hiding this comment.
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.
Hmmm, sorry, I don't understand what you mean by this. How would the exporting side preemptively know what's consuming it? |
It wouldn't. The problem is caused by a file re-exporting something. If you don't re-export anything, it's impossible to have a "barrel" approach unless you put everything in the same file. |
Why is file re-exporting a bad thing? Should we be discouraging nested barrel exports? |
It's a bad thing because it creates an otherwise nonexistent need for tree-shaking - which due to the nature of JS, can only ever do a half-assed job at deleting unused code. Yes, nobody should ever be using barrel exports, nested or otherwise, and everything should always be deep-imported from the most granular place it can be. |
} | ||
}; | ||
|
||
const isCalledIndex = filename => filename.slice(0, 5) === 'index'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const isCalledIndex = filename => filename.slice(0, 5) === 'index'; | |
const isCalledIndex = filename => path.basename(filename, path.extname(filename)) === 'index'; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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('/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const segments = sourcePath.split('/'); |
const filenamePath = context.getFilename(); | ||
const sourcePath = node.source.value; | ||
const segments = sourcePath.split('/'); | ||
const lastSegment = segments[segments.length - 1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const lastSegment = segments[segments.length - 1]; | |
const lastSegment = path.basename(filenamePath, path.extname(filenamePath)); |
Prevent shallow (barrel) imports.
Why?
Obviates the need for tree shaking.
Todo: