Initial implementation #1

petrovica · 2018-01-23T09:54:37Z

No description provided.

_ _ ____ _ ___ / \ _ __ __ _ _ _| | __ _ _ __ / ___| | |_ _| / △ \ | '_ \ / _` | | | | |/ _` | '__| | | | | | | / ___ \| | | | (_| | |_| | | (_| | | | |___| |___ | | /_/ \_\_| |_|\__, |\__,_|_|\__,_|_| \____|_____|___| |___/

petrovica · 2018-01-23T10:11:57Z

.angular-cli.json

+    "styleExt": "scss",
+    "component": {},
+    "schematics": {
+      "collection": "@nrwl/schematics"


@nrwl/schematics provides additional templates for Angular CLI code generator. The only thing I used it for is ngrx setup. More info on: https://nrwl.io/nx/guide-setting-up-ngrx

nx module also provides other helpers like effects for data loading (we probably won't use that)
https://nrwl.io/nx/guide-data-persistence

or multirepo setup (that we might actually use)
https://nrwl.io/nx/guide-nx-workspace

petrovica · 2018-01-23T10:18:55Z

e2e/app.e2e-spec.ts

@@ -0,0 +1,14 @@
+import { AppPage } from './app.po';


All *.spec.ts files are not modifed - they are generated by Angular CLI. I'll fix them later when the implementation will be "stable".

petrovica · 2018-01-23T10:23:44Z

src/app/app-routing.module.ts

+  {
+    path: 'users',
+    loadChildren: './users/users.module#UsersModule'
+  }


I guess all modules should be lazy loaded. New ngrx v4 is prepared for that quite well 👍

OPINION NEEDED:
Btw. this file was generated by the CLI. Not sure why they started using extra module for that instead simple file with route configuration, but I kept it this way. Any ideas why not to use it this way?

About lazy loading feature modules, why not? I would keep it that way by default as well.

As for having whole separate module for routing definition per feature, that has been in the official style guide for some time. I think it makes sense as it helps to separate what belongs where. Even before you basically created a module but now it is made more proper. Plus it keeps all the route specific services, guards, etc in one place.

petrovica · 2018-01-23T10:28:03Z

src/app/app.component.ts

+     <app-api-loader></app-api-loader>
+     <app-api-error-toast></app-api-error-toast>`
+})
+export class AppComponent {}


Just an entry component.

OPINION NEEDED:
I know, it's more consistent to have extra HTML template for that, but don't you think we could make some rule that really small templates that won't change much in the future could be inlined?

A agree that angular gets bloated when everything gets its own file. Or if you include them by default even if they are left empty. So my opinion if we do not want to be as strict:

No need to keep empty files if there is no upcoming short-term expectation for their usage.

Small templates could be inlined but a line-limit should be specified. Maybe up to 5 lines? Or is that too low? Up to 10 would be my absolute maximum.

Styles probably the same as templates. Maybe an added constraint might be to one selector or something.

If the above is allowed it would be great if there were some tslint plugins/rules to check it is followed correctly. Otherwise it could end up with templates of whatever size always inlined.

Personally I would keep everything separate. However this is definitely something that should be decided and agreed upon among a group of people.

I wasn't able to find any TSLint plugin regarding this topic 🙁

Well, it was just an idea to potentially enforce the rule properly and not just by word. No worries.

yuqiora

First quick review batch of some of the root files.

Having both package-lock.json and yarn.lock is a bit questionable to me. Unless the tools update both at the same time. That being said if maybe it is not such an issue as it will be used mostly by one developer, they can pick one and delete the other.

Might continue with additional comments either as separate comments or as another review batches. Depends what I find.

yuqiora · 2018-01-23T10:09:39Z

.angular-cli.json

+      "test": "test.ts",
+      "tsconfig": "tsconfig.app.json",
+      "testTsconfig": "tsconfig.spec.json",
+      "prefix": "app",


Wouldn't be my first choice but since this is just a training/sample project the default app prefix should be ok as well.

I left the default fot the application, but king of expect that if this implementation would be approved, modules in code/ would be separated in they own modules with custom prefix.

yuqiora · 2018-01-23T10:11:14Z

.angular-cli.json

+    "styleExt": "scss",
+    "component": {},
+    "schematics": {
+      "collection": "@nrwl/schematics"


👍 for this. I was actually also thinking about using these since they already incorporate ngrx / state management. Better to use available solution than reinventing the wheel.

I have to say I like the default template pretty much.

yuqiora · 2018-01-23T10:16:40Z

karma.conf.js

+    colors: true,
+    logLevel: config.LOG_INFO,
+    autoWatch: true,
+    browsers: ['Chrome'],


A detail but I would prefer the use of ChromeHeadless here. I don't like the browser window popping up.

Ok, I'll look into that when I start writing test 👍

yuqiora · 2018-01-23T10:19:44Z

package.json

+    "build": "ng build --prod",
+    "test": "ng test",
+    "lint": "ng lint",
+    "e2e": "ng e2e"


Might be worth tweaking these a little. Or at least look at the default options and consider modifying those, e.g. formatters for output, etc. But it is on low priority.

yuqiora · 2018-01-23T10:30:26Z

tsconfig.json

+      "es2017",
+      "dom"
+    ],
+    "noUnusedLocals": true


I guess this is the generated default. We likely do not need to do much else here. The only thing I would consider is to apply some additional checks that were supposed to be removed from tslint, like the noUnsedLocals here. Though not sure if there are any others like that we might want or need. Again, low priority and possibly unnecessary. At least until we have some agreed upon configuration to follow across projects.

Actually I had to add noUnsedLocals by myself. I would love to add another options for both TS and Angular (angularCompilerOptions), but it would be great if we could agree on some sane default.

yuqiora · 2018-01-23T10:31:56Z

tslint.json

+    "label-position": true,
+    "max-line-length": [
+      true,
+      140


Would lower down to 100 to follow prettier config. At the maximum I would use 120 so that it is not such a jump.

Actually this will force the developer do format long lines manually instead of letting prettier do it instead of him. Wouldn't it be better to disable this rule completely for the comfort?

Yeah, that is another option. Really depends how tslint and prettier are setup to work together.
Related:

https://github.com/alexjoverm/tslint-config-prettier

https://github.com/ikatyang/tslint-plugin-prettier

yuqiora · 2018-01-23T10:36:38Z

package.json

+    "karma-jasmine": "~1.1.0",
+    "karma-jasmine-html-reporter": "^0.2.2",
+    "ngrx-action-creator-factory": "0.0.1",
+    "prettier": "^1.10.2",


Missing any call in scripts or more importantly the lint-staged setup. Another thing to consider is how to combine with tslint. If we want to make it part of one lint process or if we want to keep them separate.

Tooling is completely untouched too (except for running prettier manually), but having that in an exmple app would be great, I'll add that 👍

petrovica · 2018-01-23T10:43:05Z

src/app/app.interfaces.ts

+import { EntityRepositoryStateRoot } from './core/entity-repository/entity-repository.interfaces';
+import { RouterState } from './core/router/router.interfaces';
+
+export type AppState = ApiState & CrudStateRoot & EntityRepositoryStateRoot & RouterState;


ngrx code that Angular CLI generates (provided by nrwl/schematics module) define name of the namespace directly in the module. Example:

users/+state/users.interfaces.ts export interface UsersState { ... } export interface UsersStateRoot { users: UsersState // <-- namespace of the module in store is chosen in here }

This means that when you import the feature module in main module, you can't decide where to put it in the store. Pros:

you are able to define all the selector of UsersModule directly in UsersModule.

Cons

you can't change the namespace of the module's store

there might be conflicts in the naming

This is pretty neat feature for feature modules (eg. users, settings), but what about modules that are imported into the application as npm deps (eg. CrudModule, EntityRepositoryModule). Best thing would probably be to define the namespace in app.module.ts for those modules, but sometimes any of those modules need to access data of another npm module - in this application CrudResolver from CrudModule needs to access data of current route that are stored in the state by RouterModule. So...

OPINION NEEDED:
Should modules like CrudModule or EntityRepositoryModule also define their namespace by themselves so they can communicate between each other when needed?

Would need to look into this a bit more to give proper answer. For now I think it is not such a bad idea to have them namespaced directly. As for conflicts, you could always isolate them a bit more or prefix the namespaces just like with components. However I am new to redux and state management overall so I don't know what other consequences there are to one or the other.

Ok, in that case we could discuss that on next JS trainings meeting. I would probably prefer namespacing them directly too. I would probably be easier, I'm just not sure if it has some real negative impact on the app.

petrovica · 2018-01-23T10:55:28Z

src/app/app.module.ts

+        api: apiReducer,
+        crud: crudReducer,
+        entityRepository: entityRepositoryReducer,
+        router: routerReducer


Again the same question - import modules into the Store/Effects like this (explicitly selecting they namespace) or create feature modules for them and choosing the namespace there?

The more I think about it the more I think that it is better for each module to define its namespace by itself.

petrovica · 2018-01-23T10:56:48Z

src/app/app.module.ts

+    !environment.production ? StoreDevtoolsModule.instrument({ maxAge: 50 }) : [],
+
+    // @nx
+    NxModule.forRoot(),


Not actually used. I you think we won't really need it, I'll remove it. One think we might use it for are tests. They have some helpers to test RxJS streams.

Looking at its code it only provides DataPersistence capability. If we do not use it and do not plan to then there is no reason to keep it here. And if we do want to use its helpers for tests then we could include it just in tests themselves.

I wanted to use DataPersistence from the start for data loading, but it does not support blocking requests like resolvers do. We might still use it for POST/PUT/DELETE requests as a helper.

petrovica · 2018-01-23T10:58:36Z

src/app/core/api/api.actions.ts

@@ -0,0 +1,14 @@
+import { ActionCreatorFactory } from 'ngrx-action-creator-factory';


I used ngrx-action-creator-factory to simplify the setup of action creators. Great thing it that it support types.

petrovica · 2018-01-23T11:03:58Z

src/app/core/api/api.decorators.ts

@@ -0,0 +1,53 @@
+import { Store } from '@ngrx/store';
+import { catchError } from 'rxjs/operators/catchError';
+import { tap } from 'rxjs/operators/tap';


Using pipeable operators from RxJS 5.5 was weird from the start but I got use to it and I like it! The only downside for now is probably a lot of imports. I had to import them one by one because Webpack cant handle tree shaking with grouped imports:
import { map, filter, scan } from 'rxjs/operators';

OPINION NEEDED:
It this preferred usage in your opinion? If yes, I probably need to clean up few last import of rxjs/add/operator/... which I used in the beginning...

I would go with the pipeable operators. I think it is easier to reason about to always import just what you really need. With the older approach of rxjs/add/operator/... it was a bit confusing when and where to import so that it works properly.

petrovica · 2018-01-23T11:08:25Z

src/app/core/api/api.decorators.ts

+import { ApiError, BusinessValidationError, NETWORK_ERROR, UNKNOWN_API_ERROR } from './api.errors';
+import { HTTP_CONFLICT } from './api.status-codes';
+
+export const apiErrorHandling = () => {


Decorators might be nice syntactic sugar for some things. This one wraps method that makes an API call (HttpClient) and handles some basic errors.

OPINION NEEDED:
It this ok?

Interesting usage and definitely not unheard of in angular/typescript. Might result in a cleaner code. Though depends how it is used. E.g. if we would want multiple decorators their order might be important. Another point to consider is how often it is used. Both for error handling and loading indicator, do we use them for every single call or just for a select few? If it is used everywhere it might be better to approach this differently. On the other hand, we build this with the intent of modularizing it and possibly using on other projects.

In short, should be ok (unless we find out that it isn't).

And sorry for nitpicking but you are not really handling errors here. Rather you translate or normalize them and throw them anyway. Actual error handling has to be done separately.

Good point, I'll rename it 👍

petrovica · 2018-01-23T11:13:21Z

src/app/core/api/api.decorators.ts

+  store: Store<any>;
+}
+
+export const withLodingIndicator = () => {


This is really weird usage of decorators, but I wanted to show it as an example. Basically what this one does is that is sets isLoading state for the application before any HTTP request fires and stops the loading when it completes.

The controversial thing in here is that it requires the class of decorated method to have HasStore interface - to have store property of type Store.

OPINION NEEDED:
(insert vomit emoticon) or tolerable hacky solution? Using service for that would be probably cleaner...
Please check the usage in one of the Resolvers.

It does seem somewhat hacky but for the moment I cannot think of a better solution if we do want to do it with decorator.

TYPO: withLodingIndicator should be withLoadingIndicator

petrovica · 2018-01-23T11:13:58Z

src/app/core/api/api.decorators.ts

+      this.store.dispatch(apiActionCreators.startLoading());
+      return oldFn
+        .apply(this, arguments)
+        .pipe(tap(() => this.store.dispatch(apiActionCreators.stopLoading())));


Bug: I actially need to use finally or something like that.

I don't think observables have something like finally. The closest thing would be waiting for actual completion of the stream on the subscription.

And to explicitly mention it, you should stop loading when the call's success and failure.

There's an operator for that: http://reactivex.io/rxjs/class/es6/Observable.js~Observable.html#instance-method-finalize

How could I have overlooked that. My mistake. But at least you know what to do there.

petrovica · 2018-01-23T11:21:44Z

src/app/core/api/api.errors.ts

@@ -0,0 +1,21 @@
+export const NETWORK_ERROR = 'networkError';


Copied from react-training app.

A bit redundant to have both public properties and their matching public method. But well, not that important.

petrovica · 2018-01-23T11:27:19Z

src/app/core/api/api.interceptor.ts

+export const API_BASE_URL = new InjectionToken<string>('API baseURL');
+
+@Injectable()
+export class APIInterceptor implements HttpInterceptor {


This does 2 things

it sets baseUrl to API server based on configration of the application

it dispatches errors to Api module

What I don't like - this is global, you can't have 2 two HttpClients with different interceptors. That's just not possible in Angular. You coud probably instantiate HttpClient by yourself and pass custom request handler, but I'm not sure about that.
So this might cause some troubles in the future (eg. with external libraries), but for now it's the easiest solution. Better solution would probably be to create custom ApiService that would wrap each request manually.

I think it should be ok like this.

petrovica · 2018-01-23T11:39:16Z

src/app/core/crud/crud.resolver.ts

+import { CrudId } from './crud.interfaces';
+import { CrudService } from './crud.service';
+
+export abstract class CrudResolver implements Resolve<CrudId | CrudId[]> {


This reducer could reduce app complexity quite a bit! How it works:

reducers in feature modules should inherit from this one

it's enough to set few properties and one method and it will take care of loading the data, normalizing it and storing everything in the store all by itself

Properties:

schema: normalizr schema which should be used to normalize data

route: route ID (used as namespace where to save normalized data)

key: resolver name (used as namespace where to save normalized data)

Methods:

params: returns array of parameters that will be passed to API call

data: fires HTTP request (or fetches the data anyhow)

data$ is a stream of data fetched from server - it automatically process the data (normalize, store). Also it's possible to get instance of the resolver anywhere by DI and call resolve() directly to reload the data (the API will probably change a bit)
I have some ideas how to improve this so it would be much easier to use one resolver to do both:

initial resolve by the router

to use the same resolver in any component to handle data fetching (eg. list of users with sorting/filtering)

I almost forgot. There's another propery:

blocking: tells the resolver if it should let router wait for the data or transition to the destination route immediately

petrovica · 2018-01-23T11:42:20Z

src/app/core/crud/crud.selectors.ts

+
+import { CrudStateRoot } from './crud.interfaces';
+
+export const getCrud = (store: Store<CrudStateRoot>) => store.select('crud');


CrudModule defines it's own selector. If we want to support this, modules have to have pre-defined namespace in the store.

petrovica · 2018-01-23T11:52:00Z

src/app/core/router/router.interfaces.ts

+  url: string;
+  params: Params;
+  queryParams: Params;
+}


On each route change, current router state is flattened and stored in the store. I think it would be better to store the route config the same way angular provides it - different ActivatedRouteSnapshot of each route so that child parent resolver can't access children's params. Also flattended router snapshot does not store route data, because handling conflicts would be pretty hard.

Another solution would be to store ActivatedRouteSnapshot for each resolver. That way it would be possible to have conflicting parameter names and route data, but some data would be duplicated in the store.

This is a bit broader topic and it might be benefitial to discuss it in person or on next meeting.

Some notes:

I would save fragment in there as well.

As for data, that is a bit questionable. Duplicates in store is a valid point. Right now I see two possible solutions. We could ignore data that goes directly to store and include only the rest but the problem would be to differentiate between them. The other option would be to make resolvers working with store to return just the normalized keys instead of the data itself.

The argument about conflicts in data between parent and children is applicable to params as well. One way to deal with it would be to require users to use more descriptive param names, e.g. /users/:userId and /skills/:skillId instead of /users/:id and /skills/:id.

Not using the whole Snapshot as is should be ok. I don't think we need everything from there.

As for flattening, have you considered to use paramsInheritanceStrategy set to 'always'?
See https://angular.io/api/router/ExtraOptions#paramsInheritanceStrategy for details. It might make things a bit easier.

The other option would be to make resolvers working with store to return just the normalized keys instead of the data itself.

CrudResolver works this way - save data to store and return only IDs. And also RouterStateSerializer can't access resolved data, so we don't need to worry about that. I saw the issue in conflicting data in routes configuration, but you're right, the same could happen with params.

I would save fragment in there as well.

fragment and data (just the data in route configuation) added

As for flattening, have you considered to use paramsInheritanceStrategy set to 'always'?

Great idea, thanks 👍

petrovica · 2018-01-23T11:52:01Z

src/app/core/router/router.serializer.ts

+
+import { FlatRouterStateSnapshot } from './router.interfaces';
+
+export class CustomRouterStateSerializer implements RouterStateSerializer<FlatRouterStateSnapshot> {


This saves route state to the store on every transition. We can change it's implementation, this one is pretty simple, since I'm not sure if we should flatten it at all.

petrovica · 2018-01-23T11:52:52Z

src/app/users/+state/users.effects.ts

+  @withLodingIndicator()
+  saveUser(user) {
+    return this.api.saveUser(user);
+  }


This is the usage of controversial withLoadingIndicator decorator I mentioned before.

Yeah, still not sure what is the best way to go about it. Where should we easily track the loading indicators and increase/decrease the counter. Not used to think in redux way so I will not be of much help yet on this matter.

I removed the need to have Store on decorated class. withLoadingIndicator will use static ApiService method. Much cleaner and less confusing to use IMO.

petrovica · 2018-01-23T11:54:57Z

src/app/users/user-detail/user-detail.resolver.ts

+    return [route.params.userId];
+  }
+
+  data(userId: CrudId) {


UserDetailResolver does not use withLoadingIndicator and has blocking = false - this way it possible to handle loading indication on in each component separately.

petrovica · 2018-01-25T09:46:07Z

src/app/core/entity-repository/entity-repository.interfaces.ts

@@ -0,0 +1,11 @@
+export interface SingleEntityRepository<T> {
+  [id: string]: T;


Unfortunatelly TypeScript does not support custom types for keys. Only string or number is allowed and even though CrudId is defined as: type CrudId = string | number;, this does not work.
I'm considering using string for this on all the places instead of CrudId. Even if it would be a number in reality, we won't do any mathematical operations with it and we'll treat it just as an identifier.

If I understand it properly, the CrudId would be in format of e.g. 1 or 'user-1' or 'user-1-skill-1' etc, coming from normalizr, right? Using strings everywhere in here would indeed make it simpler. We don't care what value lives there internally, just that we can safely use it to identify the entity. Unless I misunderstand this.

Anyway, I am for keeping ids as simple strings in all instances. It would simplify the matter.

- add `husky` and `lint-staged` - set precommit hook to run `tslint` (with no fix) and `prettier --write`

- they are not used for now but with `tslint` precommit hook we were unable to commit

- it's just a simple wrapper around `input` - it uses `FormControl` and shows errors

- also extract `ActionCreatorFactory` from 3rd party module because it does not supports `strictNullChecks`

yuqiora

The rest of app/core with app/shared and main.ts.

yuqiora · 2018-01-29T10:46:51Z

src/app/core/entity-repository/entity-repository.interfaces.ts

@@ -0,0 +1,11 @@
+export interface SingleEntityRepository<T> {
+  [id: string]: T;


If I understand it properly, the CrudId would be in format of e.g. 1 or 'user-1' or 'user-1-skill-1' etc, coming from normalizr, right? Using strings everywhere in here would indeed make it simpler. We don't care what value lives there internally, just that we can safely use it to identify the entity. Unless I misunderstand this.

Anyway, I am for keeping ids as simple strings in all instances. It would simplify the matter.

yuqiora · 2018-01-29T11:13:16Z

src/app/core/entity-repository/entity-repository.selectors.ts

+import { EntityRepositoryStateRoot } from './entity-repository.interfaces';
+
+export const getEntityRepository = (store: Store<EntityRepositoryStateRoot>) =>
+  store.select('entityRepository');


Would be better if even the namespace key for specific store state was exported as well. Easy to make a typo or not change everywhere otherwise.

yuqiora · 2018-01-29T11:17:34Z

src/app/core/entity-repository/entity-repository.service.ts

+
+  normalizeAndStore(data: any | any[], schema: any): CrudId | CrudId[] {
+    const { entities: repository, result } = normalize(data, schema);
+    this.store.dispatch(entityRepositoryActionCreators.repositoryHasChanged(repository));


The renaming of entities to repository does not serve any purpose here in my opinion.

yuqiora · 2018-01-29T12:24:07Z

src/app/core/router/router.interfaces.ts

+  url: string;
+  params: Params;
+  queryParams: Params;
+}


Some notes:

I would save fragment in there as well.

As for data, that is a bit questionable. Duplicates in store is a valid point. Right now I see two possible solutions. We could ignore data that goes directly to store and include only the rest but the problem would be to differentiate between them. The other option would be to make resolvers working with store to return just the normalized keys instead of the data itself.

The argument about conflicts in data between parent and children is applicable to params as well. One way to deal with it would be to require users to use more descriptive param names, e.g. /users/:userId and /skills/:skillId instead of /users/:id and /skills/:id.

Not using the whole Snapshot as is should be ok. I don't think we need everything from there.

As for flattening, have you considered to use paramsInheritanceStrategy set to 'always'?
See https://angular.io/api/router/ExtraOptions#paramsInheritanceStrategy for details. It might make things a bit easier.

yuqiora · 2018-01-29T12:52:25Z

src/main.ts

+}
+
+platformBrowserDynamic().bootstrapModule(AppModule)
+  .catch(err => console.log(err));


I think we should include HMR setup as well. It is helpful to have for better developer experience and trainees should not be bothered to set it up. It would be better to have it as part of the example so that they can just reuse and without trying to figure out what exactly needs to be done.

https://github.com/angular/angular-cli/wiki/stories-configure-hmr (personally I would setup dev env with HMR enabled by default and not create a separate one, testing on the other hand could have it disabled)
https://github.com/gdi2290/angular-hmr

yuqiora · 2018-01-29T13:01:21Z

Some resources for testing Angular with Jest in case we want to switch or provide the alternative:
https://www.xfive.co/blog/testing-angular-faster-jest/
https://izifortune.com/unit-testing-angular-applications-with-jest/
angular/angular#12409
angular/angular-cli#4543

- add `data` and `fragment` - use `paramsInheritanceStrategy = always`

petrovica · 2018-01-31T13:45:51Z

I managed to implement almost everything you mentioned here - things I reacted to with 👍 or added comment with more detailed explanation of implementation.

I have one idea how to fix ugly @withLoadingIndicator and then I would like to separate modules from core/(api|crud|entity-repository|...) into real modules. So main question - are you ok with current implementation in general or would you like to rewrite some of it and postpone the separtion?

yuqiora

The rest, which should be just users and maybe some other minor things that emerged along the way.

Highlights would be:

use exported keys/namespace to not repeat the strings everywhere
possibly default to OnPush change detection, should be configurable in CLI config

yuqiora · 2018-01-31T09:56:42Z

src/app/users/+state/users.actions.ts

@@ -0,0 +1,12 @@
+import { ActionCreatorFactory } from 'ngrx-action-creator-factory';


I do not like the +state in the path. Might be just the +. Though I guess this is generated by CLI/NX. So whatever.

Yes, that's generated 😐

yuqiora · 2018-01-31T10:10:17Z

src/app/users/+state/users.effects.ts

+  save$ = this.actions$
+    .ofType(usersActionTypes.SAVE_USER)
+    .pipe(
+      switchMap(({ payload: user }: { payload: User }) => this.saveUser(user)),


I need to ask once again, is the renaming really necessary? You see in the type signature that payload is User. Why rename it when it is not in conflict with anything and is used as is right afterwards?

yuqiora · 2018-01-31T10:14:17Z

src/app/core/api/api.decorators.ts

+  store: Store<any>;
+}
+
+export const withLodingIndicator = () => {


TYPO: withLodingIndicator should be withLoadingIndicator

yuqiora · 2018-01-31T10:24:54Z

src/app/users/+state/users.effects.ts

+  @withLodingIndicator()
+  saveUser(user) {
+    return this.api.saveUser(user);
+  }


Yeah, still not sure what is the best way to go about it. Where should we easily track the loading indicators and increase/decrease the counter. Not used to think in redux way so I will not be of much help yet on this matter.

yuqiora · 2018-01-31T10:40:47Z

src/app/users/+state/users.reducer.spec.ts

+import { usersReducer } from './users.reducer';
+import { usersInitialState } from './users.init';
+import { Users } from './users.interfaces';
+import { DataLoaded } from './users.actions';


Invalid imports referring either to non-existing files or exported members of those files. This is likely generated as well but I wonder if it created a reducer spec file without the reducer itself. Same with those other imports. I guess what happened is that you replaced the users.reducer with the CRUD one, probably, removed it and forgot about the test. Please cleanup properly after yourself.

As I said, I did not touch *.spec.ts files except for muting some errors.

yuqiora · 2018-01-31T15:38:46Z

src/app/users/users-list/users-list.component.html

+</div>
+<br>
+
+<router-outlet></router-outlet>


Looking ahead I guess you use this for the create user. If I remember right the original idea was to have this rendered as a modal. Do you rely on styling to make it so later on with some positioning etc? The other option would be to use e.g. dialog from material or even portal from CDK. However for a training app that might not be necessary.

As a side-note another thing I remember a bit. I think the idea was for create and edit to use the same component, right? But here they are actually wrappers around another component that drives the behavior while each wrapper is a bit different. Personally I don't think that is a problem. This does not need to and shouldn't be a direct copy of the react one. However if we plan to use the training app as a sort of example or showcase of some practices we should agree on what exactly it should be. Be it across the both react and angular or individually.

I think we should use mat-modal for that. Actually I would prefer to use Material Design components even for basic input elements and error toasts instead of implementing them manually.

React version of training app does it the same way - shared form component. And to be honest, I prefer it this way instead of just one shared component. It gives you more options for modifications. But we can change that...

I am all for using the material module. It is official (or as close to be the default as say forms or router modules) and now also keeps the same versioning I think. We do not need to automatically adopt material design itself but its utility components/services/etc would definitely be helpful. That can be done in separate PR though.

I am not against sharing the chunk that makes the most sense instead of twisting one component to fit everywhere. I agree with you that it provides more options this way in how we want to utilize it in different places.

yuqiora · 2018-01-31T15:45:42Z

src/app/users/users-routing.module.ts

+    },
+    children: [
+      {
+        path: '',


I am not currently sure but shouldn't one of these empty-path routes have the pathMatch: 'full'? Or whatever the setting is.

pathMatch: 'full' on any of those empty routes would break routing. On the first (root) one it's because it would try to math the rest of the route, but '' !== '/users'. I'm not sure why it would break the second route too, that should IMO work on leaf route.

yuqiora · 2018-01-31T15:48:37Z

src/app/users/users-routing.module.ts

+@NgModule({
+  imports: [RouterModule.forChild(routes)],
+  exports: [RouterModule],
+  providers: []


I would consider having resolvers among these providers instead of the main module. However it likely does not really matter where they are between those two.

yuqiora · 2018-01-31T15:59:11Z

src/app/shared/shared.module.ts

+@NgModule({
+  imports: [CommonModule],
+  declarations: [],
+  exports: [FormsModule, ReactiveFormsModule]


CommonModule should be exported as well so that we do not have to import it everywhere.

yuqiora · 2018-01-31T15:59:46Z

src/app/users/users.module.ts

+
+@NgModule({
+  imports: [
+    CommonModule,


CommonModule should be re-exported by SharedModule.

yuqiora · 2018-01-31T16:13:58Z

First pass of the initial state done. Now to go through the updates pushed today. This should have definitely been done in smaller steps but it is too late for that now.

Generally I think it is ok. There might be some small things to improve here and there but the basic structure and content seems fine. As for the whole ngrx / redux stuff I don't think I am the right person to comment on that. Did not try to run it in any way yet and not sure when I will have the chance. Should do so though to get a full picture.

Needs unit tests of course but please save them for separate PR. We can also discuss there whether to remain with jasmine/karma or to try jest. Styling would also be a good idea to add soon. More so to showcase the difference between global vs component styles. But again, separate PR and even the react one has its styling still open.

petrovica · 2018-01-31T16:21:15Z

This should have definitely been done in smaller steps but it is too late for that now.

Yeah, I know and sorry for that, but I had no idea how should this work from the beginning and it was more of a research.

- remove requirement to have `Store` on decorated class - fix typo

petrovica · 2018-02-01T08:23:50Z

Fixed most of the issues and tips you mentioned here. It helped clean up the code a bit 👍 .
What's next:

rewrite selectors into services
use Crud configuration resolvers have (route and key) in selectors
separate core modules into real modules

So if the architecure is ok, should we do the same Brano did with React training - create a multirepo core modules from here? Or merge this and and iterate on that?

yuqiora · 2018-01-31T16:36:16Z

package.json

@@ -51,11 +56,19 @@
    "karma-coverage-istanbul-reporter": "^1.2.1",
    "karma-jasmine": "~1.1.0",
    "karma-jasmine-html-reporter": "^0.2.2",
+    "lint-staged": "^6.0.1",
    "ngrx-action-creator-factory": "0.0.1",


Possibly no longer needed as it seems like you copied / recreated it directly in the project.

Yes, I already removed it this morning. I also rewrote the selectors in services. I'm trying push the changes in batches, but if you want, I can push it right away.

I would rather postpone improvements to separate PRs from now on. This gets to a size where it is hard to discuss. The PR became a repo of itself.

yuqiora · 2018-01-31T16:38:04Z

package.json

+    "src/**/*.ts": [
+      "precommit-lint",
+      "prettier --write",
+      "git add"


Should be ok like this for the training app. Or at least for this PR. We can explore later on how to combine lint and prettier more seamlessly and to avoid conflicts between the two later on.

yuqiora · 2018-01-31T16:41:00Z

src/app/core/ngrx-helpers/action-creator-factory.ts

+  constructor(public type: string = 'NOT_SET', public payload: T | null = null) {}
+}
+@Injectable()
+export class ActionCreatorFactory {


If this is now directly in the project the previous one should be removed from dependencies in package.json.

yuqiora · 2018-02-01T09:27:00Z

src/app/core/api/api.service.ts

+import { apiActionCreators } from './api.actions';
+
+@Injectable()
+export class ApiService {


This definitely seems like a more elegant solution. Good job. 👍

I also have an idea how to get rid of class inheritance in resolvers (extends CrudResolver + the need to call super(crud, ...)). This is pretty small change, but it might make the API even easier to use.

Sure, go ahead, if it makes it easier or more comfortable to use. But as mentioned, might be better to do in separate PR.

yuqiora · 2018-02-01T09:30:18Z

src/app/core/core.module.ts

+    {
+      provide: APP_INITIALIZER,
+      useFactory: () => () => {},
+      deps: [ApiService],


Seeing this in context it is easy to understand what it does and why it is here. But for newcomers a comment explaining this would be helpful. Or as mentioned above use OnInit of the app root component. Though the timing would likely be a bit different.

This should be hidden inside (future) ApiModule. I wanted to "autostart" this service when user imports the module.
So when we'll separate this into it's own module, it will solve itself.

Ok, makes sense.

yuqiora · 2018-02-01T09:44:31Z

src/app/core/api/api.service.ts

+  }
+
+  constructor(store: Store<any>) {
+    ApiService.requests$.subscribe(loading =>


Shouldn't we deal with unsubscribing this as well somewhere? Though the question would be where and more importantly when. We want this subscribed for the whole lifetime of the app after all. One possible option might be to use the root app component and its OnInit and OnDestroy lifecycle hooks.

And while on the topics of observables and subscribing it would be great if we could show somewhere in here the takeUntil approach which makes it much easier to manage mutliple subscriptions. Though I don't remember anymore if you actually subscribe anywhere. There is mostly the async pipe in templates.

Do we really need to unsubscribe from it? Like you said, We need this the whole liffetime of the app. When user closes the app, this whole application gets destroyed.

That is why I am asking. I am not aware of any apparent disadvantages at the moment. If it results in some memory or performance leak and such. We can continue with it as is for now I guess. We'll see how it goes.

yuqiora · 2018-02-01T09:56:32Z

src/app/shared/form-field/form-field.component.ts

+  @Input() control: FormControl | null = null;
+
+  get errors() {
+    return this.control && this.control.errors && Object.keys(this.control.errors);


Good for a start I guess. Though for better feedback the messages should be customized and user get more than just required or email or maxlength, etc. Not sure if we want something predefined already. Different apps would have different needs for error messages. And in training itself it might be a good exercise to let trainee expand on this and provide more verbose messages.

I still think we could use Material Design components directly. Wouldn't it be better (and faster)?

Not sure how this relates to the comment. Unless Material components have predefined error messages. Anyway, I mentioned elsewhere that I am for using material module. Just that we do not need to conform to material design principles/rules/etc. Those are two different things.

Oh, you're right. I forgot how mat-error works.

yuqiora · 2018-02-01T10:05:17Z

src/environments/environment.ts

@@ -5,5 +5,6 @@

 export const environment = {
  production: false,
+  hmr: true,


When we tackle unit tests we might actually need another environment for it where it is disabled. But that is for later and depends how we will approach those unit tests.

HMR might also need some additional setup to work correctly with store, see https://github.com/gdi2290/angular-hmr. CLI does not deal with store itself so their wiki likely skipped the other stuff.

I was trying to implement this yesterday. It kinda works - Webpack only sends changed code, there's no real page reload, Angular just starts the application again. This means eg. resolvers are executed again. It's much faster, but it's still not real "module reload".

I tried to use the configuration from https://github.com/gdi2290/angular-hmr , but without any change in behavior. I guess I overlooked something.

Another thing that we can look into more properly later on. For now we should focus on the important stuff. As long as we have some basic tooling in place it should be fine.

yuqiora · 2018-02-01T10:25:35Z

Generally I am ok with its overall structure. Might be better to merge and continue with other changes as separate PRs. We can discuss on the call today if anyone else has more to say.

petrovica · 2018-02-01T11:57:46Z

Last commit just cleans up package.json a bit - mainly removes ngrx-action-creator-factory. So don't worry, no new features :)

yuqiora

LGTM

At this point I think most of the issues were addressed or they were at least identified and put in queue. The basic tooling is in place and the overall structure looks sound. Unless there are any objections I would prefer to merge this and continue with additional fixes and improvements in separate PRs of more manageable size. Plus the associated discussion would be better kept in context.

angular-cli and others added 4 commits January 10, 2018 12:49

chore: initial commit from @angular/cli

8a0ab5a

_ _ ____ _ ___ / \ _ __ __ _ _ _| | __ _ _ __ / ___| | |_ _| / △ \ | '_ \ / _` | | | | |/ _` | '__| | | | | | | / ___ \| | | | (_| | |_| | | (_| | | | |___| |___ | | /_/ \_\_| |_|\__, |\__,_|_|\__,_|_| \____|_____|___| |___/

First (probably almost complete) implementation

ab52a93

Apply Prettier

98f64f6

Merge branch 'impl' into initial-implementation

17ca094

petrovica commented Jan 23, 2018

View reviewed changes

yuqiora reviewed Jan 23, 2018

View reviewed changes

petrovica commented Jan 23, 2018

View reviewed changes

petrovica requested a review from yuqiora January 23, 2018 10:44

petrovica commented Jan 23, 2018

View reviewed changes

petrovica commented Jan 25, 2018

View reviewed changes

petrovica added 3 commits January 25, 2018 14:42

Add precommit hooks (lint + prettier)

d31dd87

- add `husky` and `lint-staged` - set precommit hook to run `tslint` (with no fix) and `prettier --write`

Suppress errors in example tests

3e0db76

- they are not used for now but with `tslint` precommit hook we were unable to commit

Add form-field component and use it in Users module

7c88277

- it's just a simple wrapper around `input` - it uses `FormControl` and shows errors

Enable strictNullChecks and noImplicitReturns

ec0d88d

- also extract `ActionCreatorFactory` from 3rd party module because it does not supports `strictNullChecks`

yuqiora reviewed Jan 29, 2018

View reviewed changes

petrovica added 7 commits January 31, 2018 12:25

Remove package-lock.json file

5725854

Remove NxModule

2efd486

RouterStateSerializer changes

ce0a10d

- add `data` and `fragment` - use `paramsInheritanceStrategy = always`

Remove CrudId and replace it with string

828b5c1

Do not use arguments

80a19c5

Code cleaup

c9c457e

Add Hot Module Reload

61606a4

yuqiora reviewed Jan 31, 2018

View reviewed changes

petrovica added 8 commits February 1, 2018 08:21

Better withLoadingIndicator implementation

a306cc2

- remove requirement to have `Store` on decorated class - fix typo

Export CommonsModule in SharedModule

9f65734

Simplify code in selectors

4b87504

Use CommonModule from SharedModule

85abe19

Use OnPush change detection strategy

9837a7c

Use | async as tempVariable when possible in templates

10fb0d7

Add email validator

832b37b

Move resolvers to routing module

22c292f

yuqiora reviewed Feb 1, 2018

View reviewed changes

Cleanup package.json

0d290d0

yuqiora approved these changes Feb 1, 2018

View reviewed changes

yuqiora merged commit 4d4714b into master Feb 1, 2018

yuqiora deleted the initial-implementation branch February 1, 2018 12:53

		@@ -0,0 +1,14 @@
		import { ActionCreatorFactory } from 'ngrx-action-creator-factory';

		@@ -0,0 +1,21 @@
		export const NETWORK_ERROR = 'networkError';


		import { CrudStateRoot } from './crud.interfaces';

		export const getCrud = (store: Store<CrudStateRoot>) => store.select('crud');


		import { FlatRouterStateSnapshot } from './router.interfaces';

		export class CustomRouterStateSerializer implements RouterStateSerializer<FlatRouterStateSnapshot> {

		@@ -0,0 +1,11 @@
		export interface SingleEntityRepository<T> {
		[id: string]: T;

		@@ -0,0 +1,12 @@
		import { ActionCreatorFactory } from 'ngrx-action-creator-factory';

Initial implementation #1

Initial implementation #1

Conversation

petrovica commented Jan 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrovica Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuqiora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrovica Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrovica Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrovica Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrovica Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrovica Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

petrovica Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrovica Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrovica Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

petrovica Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrovica Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuqiora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrovica Jan 23, 2018 •

edited

Loading

petrovica Jan 23, 2018 •

edited

Loading

petrovica Jan 23, 2018 •

edited

Loading

petrovica Jan 23, 2018 •

edited

Loading

petrovica Jan 23, 2018 •

edited

Loading

petrovica Jan 23, 2018 •

edited

Loading

petrovica Jan 23, 2018 •

edited

Loading

petrovica Jan 23, 2018 •

edited

Loading

petrovica Jan 31, 2018 •

edited

Loading

petrovica Jan 23, 2018 •

edited

Loading

petrovica Jan 23, 2018 •

edited

Loading