Clean and reasonable project structure

[pavgra]

Today files in the project are quite messed up and it would be good to define strict project structure, simplifying code navigation.

Proposal is following:

1. "components" folder for shared (maximally stateless) UI things
2. "pages" / "modules" folder for app's pages stored in hierarchical way, presenting app's structure
3. use relative paths when it possible, or absolute ones across app. and create aliases in config file only for external libs
4. CSS should be written in granular way. A file per template
5. All API calls should be accumulated in "services" folder, no inlined AJAX calls or utility code blocks should take place in controllers

Naming is a subject for discussion.

Changes required to align current codebase to the described structure are going to be done in separate branch. Also the defined structure should be put into README and forced for usage during code-reviews.

@chrisknoll, @fdefalco, @anthonysena , would be grateful to receive your opinion on this.

[chrisknoll]

I think these are great ideas and will support them in any way I can.

[fdefalco]

I'm in support of these suggestions but think they should be done after the 2.4 release.

[t-abdul-basser]

@pavgra This would be very helpful refactoring effort. I agree with @fdefalco that they should be pushed to post v2.4 release though.

[anthonysena]

Making a note that we should also include the module cleanup that @chrisknoll proposed as part of #611.

[Sigfried]

Maybe this is the right place to ask: Is anyone is considering getting away from requirejs anytime soon?

I'm back to working on ATLAS and I want to use libraries written with ES6 imports. I think I can get stuff like that working with something like https://github.com/mikach/requirejs-babel, but if we might be moving toward ES6 module imports, there's no sense messing with temporary bridging kludges.

Has this been discussed lately?

[chrisknoll]

Let's focus this discussion on project structure. It doesn't matter what the module syntax is for us to clean up and address @pavgra items, and we wouldn't want to be distracted by a holy-war over module systems.

If you do want to discuss it, I think opening up a separate issue on this or starting a forum discussion on it.

I'm back to working on ATLAS and I want to use libraries written with ES6 imports.

You can do this, just use the bundled library (via CDN or otherwise). We do this with d3, so use that as an example.

[Sigfried]

No problem. Here it is: #654

[pavgra]

Since there is overall support of the proposal, the topic was discussed at recent Architecture WG (@fdefalco , @anthonysena, @Sigfried) and Odysseus is going to start working on this step-by-step:

• First, we are going to take some part of existing codebase and suggest an updated folder structure according to the principles defined above (so we'll have screenshots of "before" and "after" folder structs as an output of the action). The goal is to see not just abstract principles, but the way of how things are going to look in real life and find out whether it fits everyone
• Then, we'll take functional blocks one-by-one (e.g. everything related to concept sets, to cohort defs, to IRs, etc) and re-arrange them according to the new approach. When a block is re-arranged, we'll create a PR for it. The idea is not to collect huge diff list and then face issues with syncing with latest changes in master, but to do work step-by-step. This will also make code-reviews easier.

[johnSamilin]

@chrisknoll, @anthonysena, @fdefalco,
we updated the folder structure and started refactoring of the data sources tab, you could see it in the branch clean_and_reasonable_project_structure.
The changes are:

Before

• all the routes defined in one file
• single file for all the reports

After

• routes for each module (like data sources tab) are defined in a corresponding module (/pages/data-sources/routes)
• an atomic component for each report, chart
  a component has a common structure: view (html), controller (js), styles (less) and sub-components which should only be used in this particular component. If a sub-component also needs to be used somewhere else, it should be placed to /js/components folder
  a component should extend a base Component class (placed in providers folder) in order to unification, code reducing and fast improvements (i.e. for example, state management)
• helpers that are being used in a module are placed in a const.js file (like paths, constants and transformer functions for charts). Helpers that are being used across the app should be placed to /utils folder

[chrisknoll]

Hi @johnSamilin

Thanks for drafting this up. A few thoughts:

1. I like the structure of routes. It makes sense that each sub-page would define it's own set of routes. Good!

2. Everything is under /pages: we're going to have components that aren't pages themselves, so I don't think we'd put everything under /pages. We should probably have a root folder for pages, another folder for components, and the components under /pages will leverage the stand-alone components found in /components

3. One-folder-per-module: This is set up like a webpack project where you expect to be able to load a module via "pages/data-sources/components/reports/dashboard", but this isn't' web-pack, and every module reference must resolve to a file, not a folder, so to load that module you'd have to use "pages/data-sources/reports/dashboard/dashboard", which is awkward. I like how the components under 'charts' is structured, but I'm a little confused bout that case because wasn't our charts exported to an external library? So I don't know why those files are shown here...except I like the arrangement of those files vs. the ones under the other report sections.

With the last 2 points in mind, I've put together 2 options I'd like you to consider.

Option 1: one module per file, separate folders for template and stylesheets:

In this option, I'm putting components and pages in separate root folders. Pages will contain all the components specifically for driving the Atlas pages. components will be where we place the re-usable elements of the application.Note: I'm putting charts and reports under /components because they should be reusable things, and not specific to any one page...For example: the person report should be the same report for data sources and cohort characterization. I also have separate folders for templates and stylesheets to tuck those assets away from the main folder with the modules. To reference those assets, in the module define, you would do this:

define(['text!./templates/dashboard.html', 'less!./stylesheet/dashboard.less'], function (template) {...});

I propose this option so that you can look at the directory structure and clearly see what components are defined.

Option 2: one module per file, do not have separate folders for styleshets/templates

This folder structure is nearly identical to option 1 (from a top-level folder organization), but instead of sub-folders for stylesheet and template assets, they are now in the main folder for the component, and if we adhere to some naming conventions, the assets will be grouped together. In this way, the external assets are imported via:

define(['text!./dashboard.html', 'less!./dashboard.less'], function (template) {...});

Module ID resolution

With this folder structure, instead of this:

// some page module
define(['pages/data-sources/components/reports/dashboard/dashboard'], function () { } );

It is this:

// some page module
define(['components/reports/dashboard'], function () { } );

If we do feel like we should have sub-groups of reports, we could put that into a sub-group:

// some page module
define(['components/reports/data-sources/dashboard'], function () { } );

But I still think that we should think of reports as more atomic things that aren't tied to one specific function in the application.

-Chris

[pavgra]

@chrisknoll , I believe that the first option, which is structuring by content type, doesn't work good for big projects, especially when we do not have strict typification.

The second option looks quite similar to what was proposed in the branch, but I would combine it with the @johnSamilin 's approach:

• there are common components used all across the app, and of course, it is reasonable to store them in top-level components folder
• there are components, which are specific to some page (or a group of pages) and as for me, it sounds logical to put them into folder nested in the page's one. Otherwise, the top-level components folder will grow much faster and separation by modules (pages / group of pages) would not be so clear and obvious. In other words, if we have a page and all its dependencies in a single folder, we could easily modularize it, and it would enforce loose coupling of the app's modules (functional modules).

[johnSamilin]

@chrisknoll, thanks for the review,
as for module IDs - we could rename components/dashboard/dashboard.js to components/dashboard/main.js and thus get rid of the repetition and save the folder structure. This will make errors in console less informative though helps you stay focused when you work on a component.
Also, wasn't our charts exported to an external library? : there were, charts components here are just a markup and a wrapper on top of atlascharts call

[chrisknoll]

Hi, @johnSamilin ,
I don't think this is a viable approach. If i'm working on multiple modules (which happens often), all of my open files will all be 'main.js'? That doesn't sound reasonable. And it sound like every module we import will have a /main.js suffix on it. Also, this doesn't seem reasonable when it's not something that is required. In webpack, under the covers a module 'main' is created under the module path, but it's transparant to the user (due to the bundler resolving those). But pushing that on the developer, I don't think we should require that.

I feel what's happening here is that we're trying to make this all look like a node or webpack project structure, and we're using AMD formatted modules. In AMD world, it's one module per file (vs. one module per folder). So, since we're in the world of AMD, let's follow the model that modules resolve to a single file, and work under those rules. I think we should consider folders as 'collections of modules' (or like a java 'package').

@pavgra, I understand your point about folder specific content types (when I suggested a stylesheets and templates folder) instead, maybe we have a more generic 'resource' or 'etc' or something else that we can place all those separate assets into (if we have an objection to having the resources co-mingled with the module files).

[pavgra]

@chrisknoll , if the resource (under resource I assume stylesheet) is component specific, why should we move it far from the component? It should be tightly coupled with the component's controller and template, so should be near

[chrisknoll]

I'm thinking that resources might be shared across components (ie, an image for an icon or something like that). But my Option 2 above grouped those assets all in the same package folder so I'm fine with keeping them with the component, but remember: we'll have multiple components in a single folder, not one folder per component.

[pavgra]

Icons and similar resources - yes, no objections. I was talking about stylesheets.

So, all in all, do we agree on the following change list for the PR?

• move components, which are common for multiple modules into shared top-level "components" folder
• have top-level and module-level "resources" folders for images and similar assets
• flatten folders to exclude requires like "some-component/some-component"

@chrisknoll ?

[chrisknoll]

I think that covers it, @pavgra , but I'd definitely like to review again as the implementation moves forward, there could be another issue that's not easily observed yet. So, at the very least, I agree with those 3 items you list, but I'm not saying that's the complete list.

But hitting just those 3 items would be a great improvement over the current structure. So I'm all for it.

[pavgra]

Cool! Then @johnSamilin will do the changes and we'll see whether anything else comes out. And as we discussed earlier, the approach, which we are targeting, is to go with PRs module by module, not with an insanely huge one.

[johnSamilin]

@chrisknoll @pavgra could you review again?
I flattened folder structure and added resources folder (though it is empty for now). I also made some components globally available (heading & charts) to reuse it on other pages.
Note that not all reports had been refactored yet.

[chrisknoll]

Ok, I think this looks better. I'm still concerned with seeing an 'index.js' under the data-sources folder as well as a data-sources.js file (which would be resolved by pages/data-sources/data-sources. I think that there's still this notion that a module is a folder, not a file. Am I looking at the wrong things? the modules under classes look clean....

[pavgra]

@chrisknoll , have you seen what is in the index.js? any suggestions for better naming?

first data-sources is the module name
second data-sources is the page name

I would suggest to wrap the refactoring of single data sources module into PR, go through it with a review and merge it

[chrisknoll]

have you seen what is in the index.js? any suggestions for better naming?

No, I was just looking only at the project structure to see if it conveys the information about the project structure without needing to have to look inside the files to get the general idea (which is what the point of the folder-restructure is), and so I'm just giving my perspective based on just looking at the structure.

But I'll look into the files I have a question on and suggest names that reflect their purpose. Thanks, @pavel and @johnSamilin!

[pavgra]

@chrisknoll / @johnSamilin , another suggestion from me is to build module-local permission services. E.g. permission checks of Cohort Characterization module are not required anyway outside of the module, so keeping them encapsulated will allow to keep global AuthAPI from growing w/o a need more and more:

[anthonysena]

Closing due to age and it appears that most major concerned were addressed via PRs linked to this issue.