brandonroberts / vladivostok-sample

1 stars 0 forks source link

Impressions #2

Open wardbell opened 8 years ago

wardbell commented 8 years ago

Intro

A place for us to leave general thoughts. Not always actionable.

Overall, this seems very clean

Where to configure the router.

I see you did it in main.ts. Why? In fact, I don't think any of this belongs in main.ts

bootstrap(AppComponent, [
  provideRouter(routes),
  DialogService,
  CanDeactivateCrisisDetail
])

My sense is all of it belongs in AppComponent

Async Loading

I have no idea if this works.

Let's explore creating the CrisisCenter as an async loaded module (+crisis-center) and put its routes in a separately loaded module so we can confirm that we can configure the router independently of a lazy loaded module.

Diagnostics

We have nothing here that would help us explore how the new router facilitates monitoring the router's behavior. For example, I don't yet know how to tell

It may be in the router. I just don't know how to tap into it yet.

CanDeactivate as a service

I was skeptical. I have long felt that this should be an instance method. But your implementation convinces me that this need not be so.

More importantly, because the component instance is available to the CanDeactivate method, we have the best of both worlds. If I don't want the CanDeactivate method to know the business logic, it can call into a component's own canDeactivate in whatever manner the developer desires.

I'm sold.

DialogService.confirm returning a promise

To properly simulate async for canDeactivate we have to make this async. I ran into all kinds of trouble doing that: #7, #8, #9.

Crisis Center Issues

I'll enter most as separate issues. Many of these will be router bugs, not bugs in the application. When you confirm, we can post these as issues to vladivostok or wherever. It is easier for the moment for me to record them here.

We really need typing support. I was able to fake it in as described in issue #11