bluehalo / mean2-starter

Other
4 stars 0 forks source link

Change to absolute pathing #118

Closed Sieabah closed 7 years ago

Sieabah commented 7 years ago

Relative pathing exists where it makes sense, internal to each module.

There are bugs dealing with Eua and User related services/classes/components that cannot be loaded with absolutes. I haven't had a chance to look further into it.

ymao1 commented 7 years ago

Have you coordinated with @reblace about this refactor? I believe he's aggressively rearranging a bunch modules to support lazy loading. Not sure how far along he is, but it will likely have a bunch of conflicts with this PR.

Sieabah commented 7 years ago

Anything dealing with

Ends up having a cyclic dependency at some point which causes undefined classes. No idea why it isn't caught by TypeScript.

An example is user.class.ts which relies on Credentials. I should be able to import from app/authentication but when I try it will export all classes in authentication/*, which User is referenced a lot.

I believe a lot of the headaches will go away if we split out the classes or try to rely on not having two layers for the application. The UserService should care about users, a TeamService should care about the UserService not TeamService directly interacting with Users, the types should be implied when dealing with specific functions on the UserService.

@ymao1 Yes I was talking to him over the weekend about this.

Sieabah commented 7 years ago

I'm pretty sure I've converted all of the files that I could. Anything not using an index.ts has a cyclic dependency that isn't easily resolved with the DI system.

Sieabah commented 7 years ago

@reblace So Typescript is able to determine the cyclic dependencies fine. It's when the decorators are evaluated that causes the problem. The Angular DI system when the decorators are run cause some to be undefined due to the cyclic dependencies. It's an issue when using index.ts as every export statement is expected to finish and export (for Angular DI).

The solution is to really start to decouple the classes that have this problem.

Sieabah commented 7 years ago

@reblace To further clarify why load-order is suddenly making a difference. When loading via index.ts it exports everything before actually resolving to the parent index.ts or whatever module is requesting it. In doing so the dependencies are deferred to resolve later (cyclic). Typescript doesn't complain and neither would javascript. The issue is the decorators needing the necessary metadata at compile time which when they're executed the actual definition is deferred. So we get a nice big undefined or ? warning from the Angular DI system at runtime.

Edit: This wouldn't change tree shaking as it still breaks out what isn't used from the exports.

Sieabah commented 7 years ago

@reblace Was there anything else you wanted to add?

Sieabah commented 7 years ago

Changes Made So Far:

Sieabah commented 7 years ago

How to fix Cyclic Dependencies

Manage state through a shared top level service. Components having interdependencies creates a layer that is solved during compilation but after decorators are called.

This is only an issue when you use index.ts as every file referenced in the index.ts file you reference must finish exporting.

So for example if we have folders, A and B. A has an index which exports a1 and a2. B exports b1. If b1 imports a1: import { a1 } from '../A'; and a1 imports b1: import {b1} from '../B'; In Typescript the compiler as it's reading b1 will wait until a1 is defined, and will take into account the cyclic dependency and resolve.

The issue is decorators which are abundant in Angular2 are executed at compile time. They are also still experimental. So during the time where there is a cyclic dependency (even if indirect) the decorator will be executed and the metadata that Angular uses is unavailable which results in the DI system having the reference defined and injectable but the object injected is undefined or null.

An example of a highly cyclic dependency is AsyHttp and the User class. Before it was a direct link from AsyHttp (which a lot of components/services used) to User. Meaning almost any class had to have User exported, and since AsyHttp would have exported through 'admin/user' or 'admin', any service or module in the exported in the relevant export would also have to be exported. So by including AsyHttp in something like Teams your dependency chain results in Team -> AsyHttp -> 'admin' (index.ts) -> (User, ..., user-management) -> (..., admin-list-users.component) -> 'teams' (index.ts) -> (Team, ...). This chain results in loading almost the entire application.

So in short, Typescript doesn't have a problem, but AngularDI does. Using index.ts takes out the hard requirement of importing specific files, any interaction with services should be done through AngularDI. State should be handled with something like Redux or a giant object that is injected.

Sieabah commented 7 years ago

Why to use root alias:

So the root alias for 'app/*' that I added is to simplify importing from outside your current module. The issue without using paths is that you lock yourself into strict this file is this many folders deep. It makes reorganization later a real hassle. Take for example when you require from npm, you don't do nest your dependencies down to the implementation require('../../../node_modules/lodash/get/index.js'); You just simply do require('lodash') and rely on npm to assume since it didn't start with . you meant the node_modules folder. Also from that you expect the exported object to have what you need where you need it.

If I know I need the User class in the admin module and I'm in the teams module I can simply rely on the exported module 'app/admin' to have what I need and I don't need to worry about the underlying file structure of that module. Since it is an alias you are still free to do app/admin/user/user.class and pull in your exported user, but if you use it in conjunction with index.ts you get a very nicely packaged module layout.

So aliasing app/* I believe is unique enough, it can be whatever you want asy_app, appASY, LOCAL_APPLICATION_FOLDER_ASYMMETRIK, literally anything you want. If you're worried about accidentally pulling in app as a dependency and meaning to use that, then yes, use a different folder name. It can be named wildfire/firehawk. It can even be named app in your directory and aliased in tsconfig as apollo/*.

Sieabah commented 7 years ago

Bump