Coding conventions

[pavgra]

Background

A lot of work was done on UI refactoring (#633), many initials ides were rethought during the evaluation, a lot of new things were brought into and finally, I feel that we are ready to write down and agree on new coding conventions to make contribution requirements transparent and our codebase - consistent.

Proposal

General patterns

• Folder nesting should follow /split-by-type/split-by-domain/split-by-type/... approach
• All component containers should use ES6 classes. No methods should be defined in constructor

Project structure

/components

• UI component which is used across two or more different pages sections (folder under /pages) should be placed under components folder
• Component typically consists of controller, template and styles files; it may also include data models and component-specific utils
• Each component's controller should extend Component class
• If a component has methods that are used in a view, it should be decorated by AutoBind class (e.g. class FanceSelect extends AutoBind(Component) to preserve context for those methods
• Components should be atomic and follow 'file per component' principle. A single file shouldn't register (export) multiple custom tags

/config

• To store all configuration variables

/extensions

• /bindings - to store Knockout bindings
• /data - to store JSONs and CSVs
• /ko-extenders - to store Knockout extenders
• /plugins - to store RequireJS plugins

/pages

• each tab in nav menu should be represented by folder under /pages
• each nested folder should contain:
  • index.js which exports an object with fields title, buildRoutes, baseUrl, icon
  • routes.js which exports a function returning routes (either protected by athorization Route#AuthorizedRoute or public Route#Route)
  • const.js which exports rest url builders, constants
  • components folder that contains pages components and their nested UI components
  • services folder if there are more than 2 services
  • other split-by-type folders to store more than 2 entities of a type used across the section (e.g. models, utils, etc)
• each page should extend Page
• router params should be observed via onRouterParamsChanged

/resources

• /libs - to store external JS libraries
• /styles - to store application-level styles
• /images - to store application-level images
• /fonts - to store application-level fonts

/services

• To store application-level methods for communication with backend
• May have nested domain-based folders to store appropriate data classes

/utils

• To store utility methods which should be represented by pure functions (no side effects)

To be refactored

/data -> /resources/data
/providers -> to be removed by putting stuff into context-specific locations
/modules/circe/components -> /components/circe
/modules/conceptpicker -> /components/conceptpicker
/modules/conceptsetbuilder -> /components/conceptsetbuilder
/modules/databindings -> /extensions/bindings
/modules/job -> /services/job
/modules/plp -> /pages/plp/services
/modules/WebAPIPRovider -> /services

Libraries

• It is restricted to use jquery ajax in favor of /services/http.js
• It is highly recommended to avoid usage of jQuery in favor of Vanilla JS
• It is highly recommended to use Lodash methods instead of introducing new ones

Styling

• All style files should be coded using LESS and levereging benefits of the transpiler
• Styles should be written according to BEM methodology

Dependency management

All external libs should be installed via NPM

Build process

Webpack is a build tool to produce resulting JS and CSS bundles

[pavgra]

@chrisknoll , @anthonysena , @fdefalco , @johnSamilin , @wivern , would be great to hear your feedback and get to the agreement

[johnSamilin]

Totally agree with these proposals

[chrisknoll]

Thanks for letting me take the time to review the codebase as it has evolved so I can take an objective view about what some of these patterns mean from a code structure perspective and what challenges they might introduce that we should consider when applying them.

General Patterns

Folder nesting should follow /split-by-type/split-by-domain/split-by-type/... approach

I have a slight disagreement with this approach if the levels of the folder structure does not convey some type of meaning about what function/domain the contained modules support. For example, i wouldn't be for a structure that goes pages/iranalysis/components/view|model|dto|service/subfunction/|view|model|dto|service/subfunction. I'm being a little hyperbolic/exaggerating here, but if you take a look at pages/pathways, each level under that folder reveals additional information about what is in the functionality: at the root we have the routes, 2 services, a data object, constants, and a collection of components. Under components we see we have a browser and manager at the top level, and a group of thigns under 'tabs'. Under tabs, you see the components that support the 'tabbing' function of the UI. This isn't strictly following /split-by-type/split-by-domain/split-by-type/, but each level reveals more deail about the underlying sourcecode. In contrast, if I was to reveal a folder and it had 'view,model,dto,service' folders, that doesn't reveal anything new about what is going on in the function. So, that's my perspective on that, I hope we can find middle ground.

Project Structure

These items look good. For /config, we just need to remember that the config-local is used as a 'root configuration' so we shoudln't see various 'override-able' local-configs spread out among the sub-folders of config. the default config options can be nested beneath, but the root config overrides the children at a single point.

We have an /assets folder, I think that's replacing /resources in your proposal?

For libraries, I think we should leverage libraries like lodash if it solves a specific problem. The problem with lodash, in my opinion, is that it is extremely heavy. I'd like to know if we'd be open in creating a minimal build of lodash that just exposes the function of the library that we use. My gut tells me it would reduce the +500K (minified!) size down to something < 30K.

Agree on styling, but we have usecases where we might want to share a class of styles across components. For example: how we style axes in our graphs might be something we want to share (or just have one place to change). Right now the approach is to have one set of styles per component which would include the axis styles, and I feel we need to come up with a way to share this styling across a series of related components (without resorting to inherit all compoents that draw axis from a root component object 'ChartWithAxis'.

Dependency Management

I'm in favor of dependency management, my concern is the baggage attached to node/npm that doesn't make it well suited to browser environemnts (without additional build steps). I'm not saying I have the ideal solution, but it would be great if we could explore things like systemJS (https://github.com/systemjs/systemjs) other other systems for module loading (some are based on JSPM). In the long term, we should be able to leverage native ES6 modules in the browser (since we're pretty narrow with the browsers we support). In the short term, I'd like us to look into having a more fine-grained javascript bundling compared to our single 7MB bundle we download today. It isn't just about the size, I really feel like there are areas of Atlas javascript that should be compartmenatlized and budnled for sharing across different applciations (such as arachne), or creating a series of sharable libraries that external devs might want to leverage for themselves (like we did with AtlasCharts).

My impression is that Atlas can be broken down into 6-10 sub-bundles (such as sub-pages, generic components, services, utlities, visualizations) that could allow us to have a very streamlined download profile to minimize startup time. I haven't seen the single 7MB bundle out in the wild enough to know if it leads to 'front-loaded' startup times which are annoying (but very fast application response after that first load) vs. a very fast startup time loading just the sub-bundles required for a specific function. I completely understand non-bundled is not ideal at all (tho it does lead to only loading what you need) but i feel there should be some middle ground between 'load everything at once' vs 'load each tiny piece one at a time on demand'. I feel we have an architecture of dynamic loading of libraries, but we're not leveraging it!

Build Process

This is tied a bit to how our dependencies are managed, so some of my points were already made in the DM section. It works, but I'd like to have it more refined to support separate chunks of functionalty that makes sense structurally as well as a more refined download of the app to the client.

[chrisknoll]

On Dependencies and Build Process, please read this thread:
package-community/discussions#2

They are tackling issues that I've been trying to address using the AMD loader system that we've adopted for Atlas. It goes into great detail about the particular issues that they are trying to solve, why the node dependency resolution algorithm isn't suitable for the web, and some ideas about where the solutions may come from. The thread talks about 'de-duping' dependencies, and how people are commonly faced with multiple loads of lodash in their bundles to deal with different versions of 'left-pad' due to different semantic version specified dependencies for the different versions of the library. These are the kind of issues I want us to be very clear about how we intend to handle.

I just want to make sure we're not going to go 'all in' on a node/webpack bundling solution when there are solutions out there to solve those problems while being browser-friendly out of the box.

[anthonysena]

@chrisknoll thanks for doing all of this digging into dependency management and the build process. In an effort to help myself understand this discussion a bit more, let me attempt to summarize: we currently use AMD module loading (via requireJS) to load up JavaScript dependencies. The upside to this approach is that only the modules that are required to support a page are loaded. The downside is that it can be quite chatty between the client/server when there are a large number of required elements. A build process could be utilized here to create a single unit in JavaScript for those larger "chunks" of the application that are used. I believe this is what you are proposing above when you say:

My impression is that Atlas can be broken down into 6-10 sub-bundles (such as sub-pages, generic components, services, utlities, visualizations) that could allow us to have a very streamlined download profile to minimize startup time.

Let me know if you see any gaps in this summary. Oh and one more item to add to this list:

Code Formatting

Per #655, we'd like to adopt EditorConfig need the ability to have a consistent set of rules for formatting source code. Furthermore, we need a set of standards for our JavaScript code to ensure there are no unused variables, things are declared as const, etc. We could consider using EditorConfig with Prettier with ESLint as described in this article:

https://survivejs.com/maintenance/code-quality/code-formatting/

and here:

https://stackoverflow.com/questions/48363647/editorconfig-vs-eslint-vs-prettier-is-it-worthwhile-to-use-them-all

I welcome any thoughts on this topic - I have no practical experience with these but they are fairly popular and hope that others might have used them and can provide their feedback.

[pbr6cornell]

It seems we need to resolve this as we go into ATLAS 3.0.

[anthonysena]

Moving this to the 2.14 milestone as this proposal may require re-review and I think some of these items may have been addressed over the past few years.

[anthonysena]

Closing this issue as nearly all of this has been addressed in the project. If we need to revisit this topic, we will start from where we are currently on the project.