mgechev / angular2-style-guide

[Deprecated] Community-driven set of best practices and style guidelines for Angular 2 application development
https://mgechev.github.io/angular2-style-guide/
1.2k stars 98 forks source link

The recommended file/folder structure feels redundant and cluttered. #49

Closed NullVoxPopuli closed 8 years ago

NullVoxPopuli commented 8 years ago

Example: components/avatar.component.ts

If we are already in the components folder, we don't need to have component in the file name. We already know we are dealing with a component.

I propose the following, based on Ember's new 'pods' concept.

instead of:

├── admin
│   ├── home-dashboard.component.ts
│   ├── home-dashboard.component.html
│   ├── home-dashboard.component.css
│   ├── home-dashboard.component.spec.ts
│   ├── login.component.ts
│   ├── login.component.spec.ts
│   ├── admin.model.ts
│   ├── user-management.service.ts
│   └── order-management.service.ts

I think it should be this:

.
├── app
     ├── components
     │   ├── home
     │   │   ├── dashboard
     │   │   │   ├── component.ts
     │   │   │   ├── template.html
     │   │   │   ├── style.css
     │   │   │   ├── spec.ts
     │   ├── login
     │   │   ├── component.ts
     │   │   ├── style.css
     │   │   ├── spec.ts
     ├── models
     │   ├── admin.ts
     ├── services
        ├── user-management.ts
        └── order-management.ts

this structure allows for a couple things:

@Component({
  selector: 'path/to/and/name-of-component',
  moduleId: module.id,
  templateUrl: 'path/to/and/name-of-component/template.html',
  styleUrls: ['path/to/and/name-of-component/style.css'],
  directives: [ROUTER_DIRECTIVES]
})

\ this @Component declaration could be entirely inferred based on structure, but this would require some additional changes to angular2 to be able to support implicit declaration.

Edits:

etkirsch commented 8 years ago

I like this. Is there a reason that files are named like src/about/components/about.component.ts? We can already infer that it's an 'about component' based off of the path, especially if the proposed structure were used. I don't understand the need for such heavy redundancy.

etkirsch commented 8 years ago

You know, this actually makes it easier to tell what you're looking at because you don't have to look at the file extension beforehand. It's cumbersome to look at about.component.ts and about.component.html and only use the last 3-5 characters to know what you're actually seeing, ESPECIALLY when you have sidebars like in atom that need scrolling.

image

as opposed to the proposed

image

I think this is a major quality of life issue actually. I have started making a variant of angular2-seed at this repository just for reference. It's not finished or anything, I just wanted to show off how much nicer the structure would be.

mgechev commented 8 years ago

The suggestions make sense. I really don't like the duplications we have at the moment.

The biggest two benefits of using functionality.type.ext is that you can easily identify where are located the files you need inside your IDE/text editor with multiple tabs open + easier fuzzy search.

Actually I am not sure the style guide should go in that level of details, to me it seems that the separation by bounded contexts is enough.

NullVoxPopuli commented 8 years ago

is that you can easily identify where are located the files you need inside your IDE/text editor with multiple tabs open + easier fuzzy search.

@mgechev I totally understand the desire to do that. I know at least in Atom, fuzzy searching also works with folder names. So you can search "type functionality" to get the same result.

image

joshwiens commented 8 years ago

So was originally very much for the more verbose naming, right up to the point where the file names in tabs get cut off in something like Atom.

I would definitely prefer named components/templates over having 64 component.ts files in my application ( actual count from one production angular2 application )

All that said, it's getting a bit late in the game. If we are going to change file naming conversions and folder layouts again, let's do it and then leave the dead horse alone.

hallister commented 8 years ago

Having a separate test directory has never made any sense to me. Why overcomplicate?

├── app
│   ├── components
│   │   ├── home
│   │   │   ├── dashboard
│   │   │   │   ├── component.ts
│   │   │   │   ├── template.html
│   │   │   │   ├── spec.ts
│   │   │   │   ├── style.css
│   │   ├── login
│   │   │   ├── component.ts
│   │   │   ├── spec.ts
│   │   │   ├── style.css
│   ├── models
│   │   ├── admin.ts
│   ├── services
│       ├── user-management.ts
│       └── order-management.ts
NullVoxPopuli commented 8 years ago

Having a separate test directory has never made any sense to me.

@hallister, Your example still looks much better than the current recommended structure! :-) I could get behind a spec.ts per component directory, as long as it was just unit tests, I think..

What would you do for acceptance tests though? (I like to have unit / acceptance tests in different files)

hallister commented 8 years ago

Acceptance tests are a different beast entirely IMHO. You aren't testing components, you are testing the behaviors of a website. So I'd just say make a top level directory.

├── app
│   ├── components
...
├── e2e
...

This fits in the deployment model we currently use:

  1. Run full test spec.
  2. Build
  3. Push to QA
  4. e2e Tests
  5. Push to Prod
  6. e2e Tests
joshwiens commented 8 years ago

@NullVoxPopuli

I could get behind a spec.ts per component directory, as long as it was just unit tests, I think..

This is the way it is now, and I don't believe anyone is going to change that as it's pretty standard for Angular development ( this is how the specs are setup in the angular2-seed ).

There is significant benefit to having your unit tests local to the code it's testing which are already outlined in the "Testing" section of the style-guide.

End To End Testing / Performance Testing is another animal and usually dependent on the tool you are using. It's also probably outside the scope of this document.

NullVoxPopuli commented 8 years ago

Ok, I'm going to update my original post to reflect the test paths

localpcguy commented 8 years ago

I don't know if this is a done deal, but I can say from experience in Angular 1 apps that separating by folder and then having all of the component files (in v1 it was controller) named component.ts is a pain when you have multiple component files open in tabs, looking at diffs, find in files, and even in fuzzy find (and yes, I know fuzzy find works with folders). We have a v1 app in production (we didn't write it) with 50+ controller.js files in feature folders, and it is a royal pain working on it specifically because of that style of naming.

I would much rather see a bit more redundancy like feature/feature.component.ts. It solves all the above issues, plus the desired goals of organization, and only has a tiny expense of duplicating feature/folder names.

── app
│   ├── components
│   │   ├── home
│   │   │   ├── dashboard
│   │   │   │   ├── dashboard.cmpt.ts
│   │   │   │   ├── dashboard.tpl.html
│   │   │   │   ├── dashboard.spec.ts
│   │   │   │   ├── dashboard.style.css
│   │   ├── login
│   │   │   ├── login.cmpt.ts
│   │   │   ├── login.spec.ts
│   │   │   ├── login.style.css
│   ├── models
│   │   ├── admin.ts
│   ├── services
│       ├── user-management.ts
│       └── order-management.ts

I'm fully on-board with spec files going in the feature folders alongside the code.

roelleor commented 8 years ago

I personally really liked the contribution of @evanplaice https://github.com/mgechev/angular2-style-guide/issues/10. He suggests to use one additional layer of complexity which allows for easy reuse of features in another application. Each feature (+ one shared folder) would have its own components, models and perhaps even services.

mgechev commented 8 years ago

@localpcguy I agree with you. Also feature/feature.component.ts makes complete sense to me.

The separation by "bounded context"/"lazy-loading boundary" is essential as well. If you have:

Lets say that above we won't have cross dependencies between home, login and edit-profile.

In such case the dashboard directory can explode and the next intuitive division is by code unit type/or feature again. It doesn't make such a big of a difference whether you choose the one or another. I suggest to make this optional and dependent on the developers' personal preferences.

mgechev commented 8 years ago

This commit reflects my comment above https://github.com/mgechev/angular2-style-guide/commit/e121f986d491cde790bbe5c80577245217e062ff.

NullVoxPopuli commented 8 years ago

component.ts is a pain when you have multiple component files open in tabs

I hadn't thought about the ambiguity in tabs in the editor. hmm.

So, my goal with all this is to be able to come to some predictable layout for projects that allows the elimination of redundant, error-prone configuration, such as:

@Component({
  selector: 'path/to/and/name-of-component',
  moduleId: module.id,
  templateUrl: 'path/to/and/name-of-component/template.html',
  styleUrls: ['path/to/and/name-of-component/style.css'],
  directives: [ROUTER_DIRECTIVES]
});
export class NameOfComponent { .. }

With enough structure and convention, a component definition would become, simply (if I can borrow from Ember for this):

export default Component.extend({
  ... 
});

I know that there are an infinite number of ways to achieve an assumed component configuration, but we all need to agree on a file/folder structure.

That said, some notes on things that I think harm the directory structure:

Something I don't understand though, @mgechev in your example, you have a dashboard.model.ts. Is that a data model that is only used in the dashboard component? how often does that happen? Generally my projects have had a models folder where all models go in, because they all get used in numerous places.

Using the example from @mgechev above, maybe that could be,

assuming that everything is a component anyway, why have the word all over the place? Also, since we have file extensions to help us differentiate between files, we can know which template (html) belongs to which component, because all related files here are named the same.

mgechev commented 8 years ago

In cases your application scales and you have multiple bounded contexts (i.e. directories which as intersection have only Angular 2 and some primitives you've defined on lower level of abstraction) you want to eliminate the duplication in your lazy-loading boundaries as much as possible. dashboard.model was just an example, it could be a reducer, a service or something different i.e. such scenario is possible:

In the example above during the initial load time of the application lazily will be loaded the models bundle and after that either login or home depending on the selected view. In most cases, if models is on root level, this bundle will be reused across both bundles so it is fine. What we don't want is to include it in both bundles.

Another drawback of the lack of suffix, as mentioned above, is the hard navigation in case of multiple tabs open.


In recap, I like bounded context/lazy-loading boundary as high-level division of the application, and code unit type/feature as second level division. And I am for introducing suffixes to the file names. In this case we don't have the duplication of components/home.component.ts and we also have easier navigation inside of the project.

In all cases, these are only recommendations, in case a team disagrees with any of them they can be easily modified and adopted in different shape.

hallister commented 8 years ago

As an aside, the tabs problem several people have mentioned is more of a tooling problem than a development problem. There's already a plugin that solves this problem for ember:

https://github.com/KatalysatorAB/atom-ember-tabs

So now your tabs read application/adapter.js instead of adapter.js or deeply/nested/controller.js. This is a problem that should be solved by text editors and IDE's, not force us to use a specific naming convention.

NullVoxPopuli commented 8 years ago

thanks, @hallister, I'm going to install this for myself. :-)

localpcguy commented 8 years ago

@hallister while I respect that it's a convention in Ember, and hence see how that plugin is useful for that. But I don't agree with the idea that it's a tooling problem. I want the name of my file to explicitly describe what it is, and I don't want to make my file tabs any longer than they already are. The feature.type.ext naming is also a convention.

And if someone doesn't want to use that convention, then as mgechev said:

any of them they can be easily modified and adopted in different shape.