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

Rule proposal: Disallow barrel files #2922

Open
privatenumber opened this issue Nov 16, 2023 · 6 comments
Open

Rule proposal: Disallow barrel files #2922

privatenumber opened this issue Nov 16, 2023 · 6 comments

Comments

@privatenumber
Copy link

privatenumber commented Nov 16, 2023

Problem

Currently, there is no rule within eslint-plugin-import to disallow the usage of barrel files.

Barrel files, while convenient in certain scenarios, can introduce challenges in larger codebases, leading to maintenance issues, implicit dependencies, and potential pitfalls for developers.

Example of a barrel file:

export * from './util-a';
export * from './util-b';
export * from './util-c';

Proposal

I propose the addition of a new rule that disallow the usage of barrel files explicitly.

This rule would help teams enforce a code structure that minimizes the drawbacks associated with barrel files and promotes a more transparent and maintainable codebase.

I wasn't sure if this rule would belong in this plugin, but I think it would make sense alongside rules like no-cycle.

Why disallow barrel files?

  • Performance overhead: See Marvin Hagemeister’s blog post on this topic where he measures the overhead cost of loading 50,000 modules in Node.js.

  • Implicit dependencies: Barrel files may introduce implicit dependencies, making it harder to track dependencies and understand the code's structure. A rule against barrel files would promote explicit imports, aiding in clearer dependency management.

  • Circular dependencies: Barrel files can increase the likelihood of circular dependencies. The new rule would help prevent such dependencies, reducing potential issues during development.

  • Tooling compatibility: Some tools and bundlers may not handle barrel files optimally, hindering features like tree shaking and chunking. The rule would ensure better compatibility with various development tools.

Maintenance

  • Transparency: Avoiding barrel files can enhance code transparency by making it clearer where specific functionalities are defined. This improves the overall readability of the codebase.

  • Development overhead: Barrel files can become challenging to maintain as the project scales. The proposed rule would encourage developers to avoid relying on a central file for exporting modules, reducing the risk of overlooked updates.

  • Unused imports: Barrel files might lead to unused imports, impacting the size of the bundled code. The proposed rule would encourage developers to import only what is needed, reducing unnecessary code bloat.

Implementation

  • It's possible to detect barrel files instantaneously using es-module-lexer via Facade detection.

  • Auto fix may be possible with a disclaimer that if their barrel files are not compliant with having zero side-effects, it can introduce breakage. I think eslint-plugin-import in particular is well positioned to implement an auto-fix as it includes resolution logic.

@ljharb
Copy link
Member

ljharb commented Nov 18, 2023

Barrel files are horrifically terrible and should never be used under any circumstances.

However, you can already ban export from statements using no-restricted-syntax; the challenging part is to detect when someone imports something, and then exports it.

I can't conceive of any safe autofix since this plugin also runs on packages, so removing a barrel file could always be a breaking change. A different rule could perhaps handle forcing import paths to use the most direct path possible, meaning it would warn when you import from a barrel file, but that wouldn't be achievable until we support exports via resolve.

@privatenumber
Copy link
Author

Regarding implementation, if you don't mind skipping AST traversal via acorns, it's possible to detect barrel files instantaneously using es-module-lexer via Facade detection. Otherwise, we can just check the top level nodes to see if there's anything other than imports/exports (FWIW I don't think we need to make sure everything imported is exported as they can be depending on side-effects).

And yeah, I agree removing barrel files could introduce breaking changes. Especially when the barrel files are used to establish implicit dependencies on side-effects. I was hoping ESLint's autofix API had more nuance for the user to control the level of fix to apply. There's --fix-type, but it doesn't seem there's anything that indicates risk level. I guess the autofix isn't feasible for now.

Good consideration on the exports via resolve.
What do you think of a new rule that only supports internal paths to start (no packages)?

@ljharb
Copy link
Member

ljharb commented Nov 20, 2023

That detection wouldn’t help cover CJS files that only re-export, so i don’t think we need to add that dependency.

Also, it’s not about files that “only” re-export; the rule should ban re-exporting in general when there’s a more direct path to the value.

Autofix isn’t viable, but suggestions are; that would be safe because users choose to apply one.

Rules should only ever warn on first-party code since that’s the only code you can fix - the problem is that if you have a barrel file that re-exports something from a package, it wouldn’t be able to find the best path without exports support in resolve.

@privatenumber
Copy link
Author

privatenumber commented Nov 20, 2023

Thought about it more, I'm not sure if it a rule that enforces shortest path is something that aligns with what I'm asking for in this issue.

Shortest import path rule

There's two versions that could be implemented: one that takes side-effects into consideration and one that doesn't.

No side-effects considered

It's pretty common for singleton libraries to be setup in a separate file and re-exported:

// custom-axios.js
import axios from 'axios'
import axiosRetry from 'axios-retry'

axiosRetry(axios, { retries: 3 })

export default axios

If the shortest-path rule doesn't take side-effects into consideration, I imagine it would warn that importing from custom-axios.js should be replaced with importing from axios. But since it contains real side-effects the app is dependent on, it would be fine by me, and not behavior I'd like to propose catching in this issue.

Side-effects considered

If the shortest-path rule takes side-effects into consideration, barrel files would potentially not be caught because it could be acknowledged as a real dependency because it imports files with side-effects.

In the following example, the shortest-path rule would not error when importing the barrel file because it acknowledges that ./a.js has a side-effect that may be required for ./b.js to behave expectedly:

barrel.js

export * from './a.js'
export * from './b.js'

a.js

globalThis.a = 'a'

b.js

export const b = globalThis.a

usage.js

import { b } from './barrel.js'

However, in this issue, I'd like to propose a rule that would error on the existence of barrel.js despite the possibility of implicit side-effects dependencies that need to happen in the right order.

Also, when considering that any function could have side-effects (unless otherwise annotated), I think a lot of the best "shortest path" would actually be whats used.

A rule that catches barrel files

I'm thinking a rule that warns against using barrel files by checking if a file only includes imports/exports would be dramatically easier to implement and be even more beneficial in practice.


Rules should only ever warn on first-party code since that’s the only code you can fix

BTW I was suggesting ignoring non-relative paths within first-party code. (Not suggesting that we should lint or resolve paths in 3rd party code.)

@privatenumber
Copy link
Author

privatenumber commented Dec 5, 2023

@ljharb Would love your thoughts on how we can move forward. Pinging you in case you missed it!

@ljharb
Copy link
Member

ljharb commented Dec 5, 2023

It seems like there's really two useful things that we're discussing here.

You're proposing a rule that bans re-exports. This would not be safely autofixable because other things could be importing the re-exported values, and we can't autofix a different file while we're linting the one doing the re-exporting. This would only apply to first-party code. You can also already achieve this with no-restricted-syntax, but I acknowledge this makes config composition annoying.

I'm leaning towards a rule that checks requires and imports, and if there's a more direct path to the thing (because it's re-exported), forces you to import the more direct path. this would be autofixable, but not for node_modules until resolve has "exports" support. Then, after those warnings are fixed, no-unused-modules would catch the now unused barrel files and force you to remove them.

It seems to me like the latter path would be most successful and have the easiest migration path. What are your thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants