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

Feature Suggestion: Folder encapsulation determined by index/exports.js #2565

Open
carpben opened this issue Oct 8, 2022 · 21 comments
Open

Comments

@carpben
Copy link

carpben commented Oct 8, 2022

The problem

A popular pattern is to include an index.js in the folder, that re-exports entities from the folder. The developer declares which entities he would like to expose out of that folder. But in practice this intention isn't enforced, and index.js doesn't create encapsulation. Objects that are exported from another file in the folder could be imported in the entire project. Objects that are reexported from index.js can now be imported from both the index file, and the original file.

On the other hand plugin's no-restricted-imports rule can promote folder encapsulation, But requires special configuration. index.js for exports is such a common pattern, that configuring a boundary for each file is simply not practical.

Proposal

  • User can opt in to a index.js|exports.js folder encapsulation feature.
  • Existence of files by that name will create an encapsulated folder.

Examples of proper and improper use of this rule

Imagine a project with the following file structure:

src
   domains 
       posts 
            index.js 
            postsUtils 
            components 
                 PostBanner 
                 PostsList 
   pages
       Some Page 

And the following index.js

// src/domains/posts/index.js 
export PostBanner from "./components/PostBanner 
export PostsList from "./components/PostsList

Example of proper use of this rule

// src/pages/SomePage
import PostBanner from "../domains/Posts" 
// src/domains/components/postsList
import PostBanner from "./PostBanner" 

Example of improper use of this rule

// src/pages/SomePage
import PostBanner from "../domains/Posts/components/PostBanner" 
// src/pages/SomePage
import PostBanner from "../domains/Posts/postsUtils" 
@ljharb
Copy link
Member

ljharb commented Oct 8, 2022

A single "manifest" or "barrel" export file as you describe is indeed a common pattern, but it's a very harmful one that forces the need for treeshaking. (Separately, I've never seen "exports.js" before, only "index.js")

Things should always be deep-imported from the most granular file possible, and re-exports should be avoided. Encapsulation should (only) be achieved by extracting things to packages and using the "exports" field to restrict which files can be required/imported.

@carpben
Copy link
Author

carpben commented Oct 8, 2022

@ljharb I didn't quite get the treeshaking argument.

  • If there will not be reexports there will not be a need for treeshaking?
  • Why should we care if tree-shaking takes place? Seems like a minor cost.

@ljharb
Copy link
Member

ljharb commented Oct 8, 2022

Treeshaking is only needed when you import more than you need in the first place, because without treeshaking, your bundle size and/or application's memory footprint is needlessly larger.

Treeshaking, however, due to the nature of JS, can't ever do as good a job as "only importing what you need in the first place". So, an imperfect build step is entirely unnecessary if you simply don't "import from a big ol bag of re-exports" in the first place.

@carpben
Copy link
Author

carpben commented Oct 8, 2022

If we take this argument seriously, we should also avoid reexports in index.js package's, and instead have dedicated paths for exports.
So instead

import {someEntity} from "somePackage" 

We'd

import someEntity from "somePackage/someEntity" 

Your argument seems just as valid in this case.

@ljharb
Copy link
Member

ljharb commented Oct 8, 2022

Yes, that is exactly right, and that is the same best practice for packages. Deep imports are the way to go; the "main" should only ideally have one thing in it, and not a bag of things.

@theScottyJam
Copy link

theScottyJam commented Oct 8, 2022

Treeshaking isn't needed by every consumer of JavaScript. Some examples of where it's not needed, or where it's less important include:

  • Libraries that can only work on the server-side
  • CLI tools
  • libraries with a small public API. For example, a userland RegExpr library has a small API surface area (there's only a RegExp constructor, and maybe a couple of utility methods/functions), but has a lot of logic under the hood to power that API. Tree shaking such a library would have little to no effect.
  • Even some webpage code might not need it, if the code is being hand-crafted for a very specific webpage.

I agree that tree-shake-ability is an important principle. But it's not universally important, and I would love to have the option to enforce an es-lint rule like this on the packages where I deem it to be less important. Especially if the only alternative is to "make a ton of tiny packages", that's not very ergonomic, I'd rather just use index.js files without the enforcement.

(I'd also point out that anyone using class syntax is also, to my understanding, killing tree-shakability. So, if we really find this to be valuable, does that make classes an anti-pattern in JavaScript, and it would be preferable to not have es-lint rules to help guide us to make "good classes" either?)

@ljharb
Copy link
Member

ljharb commented Oct 8, 2022

On the serverside or as a CLI, it's about application memory footprint - the cost is paid everywhere.

@ljharb
Copy link
Member

ljharb commented Oct 8, 2022

I don't see how class syntax is an issue; if you're using a class, then by definition no parts of it are separable, otherwise you wouldn't be smooshing them all into the same class.

@theScottyJam
Copy link

All public methods on a class are related to the data encapsulated by the class, but that doesn't mean the consumer depends on the entire public API. I, for example, might use a third-party Map class, but only use the constructor, .get(), and .set() methods. Everything else the class provides could be tossed, and my code would still function exactly the same way.

To make a class more tree-shaking friendly, we could get into the habit of making all methods on the class static, and you're required to pass in your instance as the first parameter to the method. Tooling would be able to easily tell which methods you depend on and which ones you don't, since you're always statically referencing them.

Granted, the amount of value you can get from tree-shaking methods from a class is much less than the value of tree-shaking whole groups of modules. So, perhaps that's partly why such a practice isn't wide-spread. But, it still could be done.

On the serverside or as a CLI, it's about application memory footprint - the cost is paid everywhere.

This is true. I guess I'm coming from my own work experience where we currently have our API layer built out in node. There's currently no need for tree-shaking the API layer, as every line of code written is intended to go into production. And moving logic out into a ton of tiny packages in order to be tree-shaking friendly in the future, "just in case", would be a ton of extra work. There would be a fair amount of work to do this initial change, and work to manage those packages, along with maintenance work due to the fact that the shape of the software would become much more rigid (it's trivial to split apart or rename folders. It's much more difficult to do so with packages).

So, it seems much more wise for people like us to just stick with the index.js "standard". Perhaps, if there was an alternative solution available that didn't have such a steep cost, it would make more sense to label the "index.js" practice as something to stay away from, as there should be no reason to use it. And, perhaps that brings us back to this ES forms thread that birthed this feature request, where maybe, as is being discussed there, we can find more ergonomic ways to encapsulate multiple modules in native JavaScript without having to resort to separate packages or using index.js.

@carpben
Copy link
Author

carpben commented Oct 9, 2022

Treeshaking is only needed when you import more than you need in the first place

@ljharb, let's say you import only what you need:

import {someFunction} from "./someFolder/someFile" 

In this case someFunction is imported directly from the file where it was declared. But, as often is the case, someFile has other JS entities as well. Isn't it the case that the entire file will be added to the bundle, and parsed by the runner?
What I'm trying to show here, is that even when we use `"deep-imports", and import an entity directly from where the entity is declared, tree shaking is still helpful.

@ljharb
Copy link
Member

ljharb commented Oct 9, 2022

That shouldn’t be the case :-) someFile should only have one thing in it, to ensure that nothing unnecessary is evaluated when importing that thing.

@carpben
Copy link
Author

carpben commented Oct 9, 2022

@ljharb you are talking about a reality of very small files. It's a common practice to bunch small entities together in files according to domain.
According to this approach, if we never want to import the unnecessary, we'll have many one line files.
Imagine a const

export const SOME_PROPERTY = "some-value" 

This will be in it's own file for the sake of not importing anything that is unnecessary.

I'm not sure if minimizing bundle size and memory print is the most important concern developers should work by. Even if tree-shaking isn't perfect, maybe it's a good enough solution and can allow us developers to develop without worrying about it.

@ljharb
Copy link
Member

ljharb commented Oct 9, 2022

Common doesn’t mean good. Folders, not files, are the proper way to group related things.

@carpben
Copy link
Author

carpben commented Oct 9, 2022

I'm trying to understand what it means.
It basically means that each module should only have one export.
Instead of a module with a few functions, each function should have it's own module.
It might actually harm encapsulation, because those functions might use another function which could be hidden from the rest of the application. Now we need to export that function as well.

@ljharb
Copy link
Member

ljharb commented Oct 9, 2022

Yes, that's exactly right, and no, it doesn't harm encapsulation, since "packages" and "closures", not "files", are the only encapsulation layer that's important.

@carpben
Copy link
Author

carpben commented Oct 9, 2022

I guess this approach is doable in the era of monorepos.

@ljharb
Copy link
Member

ljharb commented Oct 9, 2022

Exactly! You can’t have encapsulation with one monolithic codebase unless it’s split into separate packages - which can use the exports field.

@carpben
Copy link
Author

carpben commented Oct 10, 2022

Not sure about this approach.

  • Having a special path for each package export.
  • Having a special file for each export.
    Those are non trivial constraints, that are not related to better code quality, but to a certain performance optimization, which in most cases is not needed.
    I would guess the performance gain of this approach over tree shaking is marginal. Is it not the case?
    I suspect that project that need this kind of optimization are the exception, not the typical use case. On the other hand this approach does require more dev effort.

@ljharb
Copy link
Member

ljharb commented Oct 10, 2022

They’re absolutely related to better code quality - things that are maximally separate are easier to understand and maintain.

@theScottyJam
Copy link

things that are maximally separate are easier to understand and maintain.

As with any principle, I would assume that there should be a balance to this, no?

A major part of programming is organization. If a function feels bloated and cluttered, split it up into multiple functions, but note that it'll make the overall module feel more cluttered. And it's true that splitting a module up into a folder of single-function modules would certainly help make the contents of the modules feel less cluttered, but, the clutter doesn't just disappear, rather, we've just moved it out of the module and into the folder structure, leaving us with a more cluttered folder structure. And, again, we can remove the clutter of a folder structure by splitting it up into packages, but now we're increasing how cluttered the dependency structure is between all of your packages. None of these decluttering movements are ever free. And what happens when the dependency structure between packages becomes too cluttered and it's difficult to track which packages depend on what? There's nowhere left to move the clutter. On the extreme side, imagine the difference between traversing and maintaining a single project with 1000 modules, and 1000 interdependent micro packages.

IMO, the best thing to do is to strike a balance. When a module is cluttered, split it up. If a particular folder doesn't contain much, perhaps it's best to join it into a module, to help reduce the clutter in the folder structure. And, as far as I've seen, this seems to be a fairly common approach, even if it's not explicitly spelled out, people seem to naturally play the clutter game like this, shuffling it around whenever it feels needed.

@ljharb
Copy link
Member

ljharb commented Oct 11, 2022

While I agree there's no objective hard line, in practice I've found that stating "one function per file by default, except where you disagree" leads to a much better outcome than "it's totally fine to put a bunch of crap in one file".

In other words, principles are most effective when they're phrased strongly enough that newcomers err on the side of the most fixable mistakes, and veterans who deviate know why they're doing so, and that they're doing so, and can justify it.

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

3 participants