Skip to content
This repository has been archived by the owner on May 7, 2021. It is now read-only.

Remove store layer #146

Open
pmuir opened this issue Mar 19, 2017 · 7 comments
Open

Remove store layer #146

pmuir opened this issue Mar 19, 2017 · 7 comments

Comments

@pmuir
Copy link
Contributor

pmuir commented Mar 19, 2017

The store layer is not compatible with reactive programming.

@jstrachan
Copy link
Contributor

in what way? :)

@jstrachan
Copy link
Contributor

jstrachan commented Mar 20, 2017

the store just provides Observble<T> for some entities and an Observable<boolean> loading indicator together with the API to mutate entities - what else did you have in mind?

@pmuir
Copy link
Contributor Author

pmuir commented Mar 20, 2017

I don't like storing the observables like that in an singleton, it feels very dangerous to me, I would prefer the method to just return the stream - it feels to me like trying to jam a request scope in, and after many years of building request scopes, I think they aren't good.

I'm also not a huge fan of deep object hierarchies so I would probably just collapse the store layer in to the service layer making both stateless (well, session/app scope is fine, just not a pseudo-request).

@jstrachan
Copy link
Contributor

jstrachan commented Mar 21, 2017

They're not singletons btw - just things you inject into components; but yeah I don't like the name 'store' at all really either :)

But we need a name for the objects which compose other service/streams to make composite view/stream/thingies. Naming is hard though!

BTW its kinda hard to pass in things like 'namespace' as parameters into the service layer as the ActiveRoute parameters are all observables too; so you really do need some object that composes ActiveRoute with other streams to make the right requests etc. Not sure the right name though!

For example we should have a thing that contains an Obervable<Pod> (with loading Observable) for the current route's namespace' as a thing you can just inject into any component. Not sure what to call the objects that join the route parameter observables with other kubernetes services into one or more observables though?

@jstrachan
Copy link
Contributor

Maybe we reuse DI terminology - we're really just referring to Observable producers. Little objects you can inject into your component which own/manage one or more observables?

e.g. someting like...

export class MyComponent {
  constructor(protected podsProducer: ObservablePodsProducer) {}

 ....
 Observable<Pods>pods  = this.podsProducer.entities;
 Observable<boolean>  loading = this.podsProducer.loading;

then the ObservablePodsProducer hides all the ActiveRoute observing to watch the current namespace and the use of the service layer.

I guess another approach to the ActiveRoute observable is to try inject the Namespace into DI using router centric dependency injection? Never tried using that angular 2 feature yet - but it might simplify having to observe parameters?

@pmuir
Copy link
Contributor Author

pmuir commented Mar 21, 2017

  • They are singletons in effect because that is how injection in Angular works - https://angular.io/docs/ts/latest/guide/dependency-injection.html#!#singleton-services
  • Agreed, we need a way to compose streams, I just don't think they should be statefuf - when I use the word service, I mean something that is stateless. As for naming, we could just call them coposers or something like that, or a composite service? Actually producers is really good.
  • @dgutride has code that will get the params without them being observables -
  • Agreed on using composition to be able to inject the current route's namespace. I would probably call that the 'namespaces service' to be consistent with what I did in -ui etc., and inject that. I would propose an API like:
export class Namespaces {

  current(): Observable<Namespace> {
    throw new Error('No provider is registered for Namesapces');
  }
}
export class NamespacesService extends Namespaces {   

  constructor() {}

  current(): Observable<Namespace> {
    return actuallyLoadTheNameSpaceForTheCurrentRoute();
  }

}

And then you can pass to the 'service layer' pipelinesStore.loadAll(namespaces.current());

WDYT?

@jstrachan
Copy link
Contributor

sounds good

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants