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 new rule module-boundary #2318

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

artursvonda
Copy link

This rule is somewhat similar to no-internal-modules but doesn't require additional setup.

The basics of the rule are very simple – if one of the parent directories resolve to a module (usually, means directory contains index file), you should be importing from that module. This allows for setting up boundary and disallow deep imports where they aren't desired.

This could be merged into no-internal-modules rule as an option as well, I welcome feedback.

The basics of the rule are very simple – if one of the parent directories resolve to a module (usually, means directory contains index file), you should be importing from that module. This allows for setting up boundary and disallow deep imports where they aren't desired.
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Every file is a module, every module is a file - i think the term you’re looking for here is “package”?

“contains an index file” in no way indicates something is a package. Folders often have an index file (I’d argue they always should) and i have plenty of packages that don’t have one at all.

@artursvonda
Copy link
Author

@ljharb, good point on naming, definitely could change. We could even be very specific in name that this is about index-as-package-boundary..

“contains an index file” in no way indicates something is a package. Folders often have an index file (I’d argue they always should) and i have plenty of packages that don’t have one at all.

This is true which is why this probably isn't an universal rule and doesn't suit everyone. But I find it a good rule of thumb on how to manage boundaries in JavsScript code. This is sort of opt-in that from now on "index" file will indicate package, boundary of which should not be crossed.

Another option to solve issue of accessing private modules would be marking them with _ prefix (or something similar) but I personally am not fan of that.

My question then is – do you think this this rule could belong here or I best publish this in a separate package?

@ljharb
Copy link
Member

ljharb commented Dec 4, 2021

Can you help me understand why it’s a good rule of thumb? Deep imports are objectively superior to always importing from a manifest file, since it obviates the need for treeshaking.

I completely agree that a mere convention for privacy is actively harmful; if it’s reachable it’s public. A package uses the “exports” field to restrict private things, but relative folders indeed don’t have the same protection available.

@artursvonda
Copy link
Author

Can you help me understand why it’s a good rule of thumb? Deep imports are objectively superior to always importing from a manifest file, since it obviates the need for treeshaking.

I think I should've made the context a bit clearer. What you're saying is definitely true for libraries. And this probably isn't very useful for producing 3rd party packages. When producing this, I was mainly concerned with large web monolith style apps. In our use case, we want to be able to define boundaries around packages easily and in scalable way so we can force developer to think, what's "public" (can be used by users of the package) and what can be easily refactored without worry of breaking code of others. In other words, we find it useful to be able to define what's public and what's private module within specific package without manually defining all patterns (like one would use with no-internal-modules). One example of that is we break our code down into feature packages but we don't want other packages to do deep imports into other packages. And packages might be nested. And we don't necessary want to have directory pattern (like nesting packages inside packages dir) because that in our opinion adds unnecessary level.

Going back to original description, the motivation is similar to that of no-internal-modules but provides other means to achieve the same.

@ljharb
Copy link
Member

ljharb commented Dec 4, 2021

Right - I’m saying that especially in large monolithic apps, deep imports are objectively better and will result in smaller bundle sizes than treeshaking is capable of producing.

@artursvonda
Copy link
Author

That's true, but I think that's at the expanse of maintainability and encapsulation which at least to our team are very important. And with proper tree-shaking there shouldn't be any issues with producing correctly sized bundles.

I don't think this should be default rule for everyone but it looks to me like it fits in with goals of this plugin, given no-internal-modules is already part of it.

@ljharb
Copy link
Member

ljharb commented Dec 5, 2021

There is no tree-shaking that can do as good a job as “only import what you need in the first place” - the limits of the JS language speak to that.

A rule that limits what you can import from where is a fit for this plugin - I’m not convinced that this particular boundary definition would be helpful, since it encourages what i consider to be a harmful practice (relying on manifest/barrel export files)

@artursvonda
Copy link
Author

Fair enough. Mind pointing me in direction where I could read more about these limitation? The only ones I'm aware of are related to import * and export * or when unshakable transforms are used to transform modern code into legacy compatible code. Otherwise, with properly set up bundler, circular dependency checks and proper imports and manifest files, I'm not aware of any inherent limitations to tree-shaking.

Also, maybe I'm approaching this wrong. Are you aware of other lightweight ways to limit access to package internals?

@ljharb
Copy link
Member

ljharb commented Dec 5, 2021

The best way is to use “exports”, and in a monolithic app, break up the monolith into separate actually-published (even internally) packages.

@aladdin-add
Copy link
Contributor

I had written a similar rule for our private repo. the background: we split the app to several sub-modules:

~/app/src
├── common
├── pages
|  ├── moduel1
|  ├── module2
|  ├── ...
  • import common in module1/module2: ✅allowed
  • import module1 in module2 ❌not allowed.

What do you think make the rule configurable - allowing users to define what's a module-boundary?

@artursvonda
Copy link
Author

I had written a similar rule for our private repo. the background: we split the app to several sub-modules:

~/app/src
├── common
├── pages
|  ├── moduel1
|  ├── module2
|  ├── ...
  • import common in module1/module2: ✅allowed
  • import module1 in module2 ❌not allowed.

What do you think make the rule configurable - allowing users to define what's a module-boundary?

I think you could use https://github.com/javierbrea/eslint-plugin-boundaries plugin for that. It allows defining packages using patterns and defining relationships between them. We use it for similar purpose.

@aladdin-add
Copy link
Contributor

👍 looks great, thanks!

@jacobrask
Copy link

@ljharb are there any existing rules in the plugin that could be used to discourage barrel files?

@joshuason joshuason mentioned this pull request Mar 22, 2022
4 tasks
@ljharb
Copy link
Member

ljharb commented Jun 30, 2022

@jacobrask no, i don't think there's an existing rule for that. I'd be interested in adding one, though.

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

Successfully merging this pull request may close these issues.

None yet

4 participants