Skip to content
This repository was archived by the owner on Dec 4, 2017. It is now read-only.

StyleGuide 04-10 - Barrels #1301

Closed
bodiddlie opened this issue May 5, 2016 · 26 comments
Closed

StyleGuide 04-10 - Barrels #1301

bodiddlie opened this issue May 5, 2016 · 26 comments

Comments

@bodiddlie
Copy link

bodiddlie commented May 5, 2016

Since SystemJS doesn't use node module resolution, but angular2 (or rather the ts config we use) does, the barrel import syntax requires either changes to the imports, or adding entries in systemjs.config.js.

If you have an app/shared/index.ts folder and do import { SharedComponent } from '../shared'; somewhere, the default SystemJS setup from the tutorials will end up throwing a 404 because it tries to load ../shared.js.

If you change the import to be from '../shared/index, or add app/shared to the packageNames in systemjs.config.js, then it works.

@linusbrolin
Copy link

I just found this out the hard way, after I went through all my code and added barrel imports everywhere, according to the Angular2 Style Guide.
Having to add every single folder with an index.ts file to system.config.js seems like such an unnecessary headache and a poor way to build apps.

I guess I'm going with the '../shared/index' approach for now, but it takes away a bit of the point of barrel imports (shorter, simpler import lines).

@jmcooper
Copy link

@johnpapa I'm curious if you've seen this behavior also. I'm adding the cookbook recipe for barrels and running into this same behavior. It seems that the only solution is to add /index to the end of the import (i.e. import { Hero } from ./heroes/index) or to add app/heroes to the list of packages in systemjs.config.js. Neither of those is mentioned in the style guide, so I'm curious if you found another solution.

@bodiddlie
Copy link
Author

It looks like the angular-cli generators add the barrel file to the systemjs.config.js file, so maybe just including a bit of info that if you're using systemjs, this is necessary?

@linusbrolin
Copy link

Seems like a poor design decision honestly.
Having to manually map (almost) every component in systemjs.config.js seems like one big step backwards.
If this is the recommended way to structure Ng2 apps, then a better solution would be for Ng2 to extend SystemJs with a small shim to provide support for Node module resolution, so that no manual config has to be done.

@Foxandxss
Copy link
Member

We are going to move this recommendation from Do to Consider.

The CLI adds the entries for yourself and Webpack support this out of the box.

@johnpapa
Copy link
Contributor

johnpapa commented May 19, 2016

Here is some reasoning here on the problem we are addressing. The number of times we import something is very large in an app. We write import { ... } from ... a lot. Those import statements add up and they can get long. There are a few things we do to make them both less numerous (creating arrays of modules) and to make them shorter (barrels). These add up significantly over time to reduce the code we write and make the app much more readable.

For example, here is how we would import these 4 services in a shared folder without any barrels. Notice spinner and toast are in their own nested folders under shared, too.

import { ExceptionService } from '../app/shared/exception.service';
import { MessageService } from '../app/shared/message.service';
import { SpinnerService } from '../app/shared/spinner/spinner.service';
import { ToastService } from '../app/shared/toast/toast.service';

This is fairly common and the larger an app gets, the more imports we have. Can we do better? If we add barrels with the index.ts file in each folder that exports the symbols, we can now do this:

import { ExceptionService, MessageService } from '../app/shared/index';
import { SpinnerService } from '../app/shared/spinner/index';
import { ToastService } from '../app/shared/toast/index';

That is shorter ... but what if we now use barrels to re-export form the nested folders' barrels too? Then we can make it a single line like this

vs

import { ExceptionService, MessageService, SpinnerService, ToastService } from '../app/shared/index';

And if we are willing to add a map for the folders in systemjs.config.js we can eliminate the need to say /index like this

vs

import { ExceptionService, MessageService, SpinnerService, ToastService } from '../app/shared';

And if we use the CLI it adds the map for us.

The only Do here is be consistent with whatever technique you want (which will be added to the guide).

In short:

  1. The CLI adds the barrel mapping for us. So we do nothing and we get to say ./shared
  2. If we don't use CLI, we map it once in systemjs config, then every import is shorter
  3. If we don't like it, we don't do it and instead do ./shared/hero.service

@jmcooper
Copy link

I agree, John. It's too bad that SystemJs doesn't have an option recognize /index by default (although there's probably a good reason that I'm not aware of). But even without that, adding the barrel to the config once seems better than the extra import statements everywhere. I like the idea of the style guide suggesting consistency and allowing developers to decide on whether to add the package or add /index to their imports. Having said that, I tend to think that creating barrels to consolidate multiple imports in a package should be a Do, not just a Consider and adding the package to the SystemJs config could be a Consider.

On a related note, when a folder only contains a single exportable item, perhaps making a package out of that folder should be a Consider since it really is just changing your import from something like ./hero-detail/hero-detail.component to ./hero-detail and not consolidating multiple imports. Or would you consider this inconsistent if you are doing this in addition to using barrels like ./shared? I admit I'm on the fence on this one, but not sure if it's worth adding a package to the SystemJs config in that case.

@Foxandxss
Copy link
Member

I like all your points there @jmcooper.

One of the reasons of doing a barrel for a single export is for consistency as you said. It is better not to have some imports on folder and some imports on direct files.

@johnpapa
Copy link
Contributor

johnpapa commented Jun 1, 2016

The real issue here is to see if there is a way to easily omit /index. I propose we add something to the style guide to explain tht the CLI does this for us, but if you don;t use the CLI then you need to add this as a default or import from /index.

@Foxandxss do you agree? if so, can you make the change and we can close this?

@mgechev
Copy link
Member

mgechev commented Jun 3, 2016

@bodiddlie there's an open issue related to this topic in the SystemJS issue tracker systemjs/systemjs#1164

@MrLoh
Copy link

MrLoh commented Jul 11, 2016

just including a function that goes through the sub folders in app and adds them to the packages object in systemjs.config.js would also be a nice option.

@chapati23
Copy link

I have a big problem with barrels in its current form: The order seems to matter. There's an issue on stackoverflow as well.

For example, if I have a barrel like this:

export * from 'my.component' // using my.service via DI
export * from 'my.service'

Then I get an error EXCEPTION: Can't resolve all parameters for MyComponent: (?).
To fix it, I have to export the service before the component:

export * from 'my.service'
export * from 'my.component' // using my.service via DI

I see this as problematic because:

  • It's not documented on angular.io in the barrels-section
  • The error message doesn't give you any help in fixing the issue
  • Implicit order rules are a code smell IMO (especially when undocumented)

@mgechev
Copy link
Member

mgechev commented Jul 19, 2016

@chapati23 I suppose this issue is more related to forwardRef rather than barrels.

@chapati23
Copy link

thanks @mgechev, i've read the forwardRef docs as well as the respective DI cookbook paragraph.

I don't understand this 100% but I guess if my.component uses either a service via DI or another component via directives from the same barrel, then they have to be exported before my.component, is that correct?

@mgechev
Copy link
Member

mgechev commented Jul 19, 2016

It's better to move this discussion to gitter.

@awerlang
Copy link

@chapati23 inside my.component.ts, import service from ./my.service.ts, not from ./index. An index.ts is for the outside world only, otherwise load order would matter and it should not.

@chapati23
Copy link

chapati23 commented Jul 21, 2016

that's what I ended up doing, @awerlang. and that's exactly where I feel it should be communicated more clearly what barrels are for and what barrels are not for.

because in my last react-project and in the node-project before that we used barrels internally as well which (at least for node) seems to be a common practice. so i think a little explanation box with some caveats on barrels wouldn't hurt

/edit:
improving error messages is discussed over here: angular/angular#9334 (comment)

general barrel-order issue:
angular/angular-cli#1282

@colinmorris
Copy link

I wonder if 4-10 should be removed entirely as a recommendation.

I would expect the recommendations in the documentation to represent a "golden path". So it's surprising that by using the loader recommended in the QuickStart guide and carefully following the style guide's recommendations on directory structure, I end up with broken code.

Shorter/fewer imports are nice, but to make barrel imports (as described in 4-10) work, I had to add more lines of code than I saved. Namely:

  • I need to maintain an ever-growing list of barrels in my systemjs.config file, and write a bit of glue code (see this StackOverflow answer for example).
  • I need to add a bunch of ad-hoc forwardRef annotations to my code to avoid spurious circular dependencies. And it won't be obvious that I need to add one until I see a runtime error about it.

Even if it was netting out to less code, I don't think it would be worth the added complexity and fragility.

Also, I believe the circular dependency issue is worse than #9344 suggests, in that it's not just a problem of sensitivity to ordering. This reddit post has a nice example of a false circular dependency that (AFAICT) will manifest regardless of any reordering of exports in the barrel files.

@Foxandxss
Copy link
Member

This is going to be reviewed again soon when we update the style guide for RC5.

@JohannesHoppe
Copy link

FYI
https://gitter.im/angular/angular?at=57b370bb7ce08cec69df3ba1

@brandonroberts Aug. 16 22:00
The style guide is being updated and will likely not recommend barrels anymore

Latest angular-cli does not create barrels anymore, too.

JohannesHoppe added a commit to angular-buch/book-monkey2 that referenced this issue Sep 4, 2016
1. i did not found any barrels in the latest angular-cli blueprints
2. see angular/angular.io#1301 (comment)
@JohannesRudolph
Copy link

Unfortuantely I find ngModules a poor replacement for barrels (because ngModules != typescript modules). I have so far found the following tactic working well, when applied consistently:

  • intra-module (inside) imports need to reference individual files in import
  • a module definition references only the barrel file (so that the barrel API surface = ngModule surface)
  • a barrel file never exports an ngModule
  • inter-module (outside) dependencies only references other module's code via the module's barrel (ensure that ngModule dependecies = barrel dependencies)

This way allows me to think of the barell file like a *.d.ts file of the whole module. It has resolved the circular dependency issues I was having. I really like having a barrel file for shared modules as it makes my other code considerably cleaner. YMMV.

@zoechi
Copy link

zoechi commented Nov 4, 2016

NgModule is not supposed to be a replacement for barrels. These two are entirely unrelated. Barrels are for TypeScript imports and NgModule is for lazy loading.

@JohannesHoppe
Copy link

.... aaaand to set up the DI! ;-)

@JohannesRudolph
Copy link

I understand as much, hence my suggestion for working with barrel files while avoiding some common associated issues. I think it's not a good idea to flat out discourage their use.

@michaeljota
Copy link

I think I maybe late for the discussion, but I like barrels. Not only I like barrels, but [email protected] allows to do awesome things with it, using the Advance Path Resolution. The APR allows to load files and barrel folders importing from root using module syntax. Defining a baseUrl property in the tsconfig file, you can load files from local root like you were loading from a module. ex:

Instead of
import { SomeModel } from '../../../../models/some-model/some-model.ts (Without barrel)

You would
import { SomeModel } from 'app/models/some-model/some-model.ts (Without barrel)
or just
import { SomeModel } from 'app/models/some-model (With barrel)

Giving a folder structure like:

src
 |- app
    |- models
    |- components using models
       |- ... somewhere you load the model

It's easy to read, and easy to maintain. I know the feature can be use without barrels, but I didn't even try this with SystemJS, I don't know if this works with this, and I would say it does not.

is just a Code Style, and it does not suppose to be followed blind, but it is really important for us as developers to have a consistent Code Style, and for this to evaluate the benefits about drop the suggestion of a useful feature.

I think this is only a SystemJS bug, and it was being tracked, and ultimately fixed as systemjs/systemjs#1164 (But I don't know if it was really about loading barrel folders).

At the end, I think that instead of dropping the use of barrels folders, you should consider to drop the suggestion of using SystemJS as a build system. But as I said, I don't know if it had being updated to APR an others TypeScript new features.

@wardbell
Copy link
Contributor

wardbell commented Dec 20, 2016

@michaeljota We all like barrels. What has changed is that they are now considered a matter of personal style, not Angular style.

There was a time when we thought they would be almost essential for aggregating access to components. After modules were introduced, that purpose melted away. They still have utility for aggregating model and service classes; I'm sure they are useful elsewhere too.

SystemJS is not a factor in this decision. Barrels and SystemJS get along fine.

Keep using barrels as you find them helpful. We have decided not to restore barrels to the Style Guide. This issue is closed.

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

No branches or pull requests