Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove tight coupling with Http api #8

Closed
thekalinga opened this issue Jan 25, 2017 · 4 comments
Closed

Remove tight coupling with Http api #8

thekalinga opened this issue Jan 25, 2017 · 4 comments

Comments

@thekalinga
Copy link

Loading should be an activity on its own.

Currently, this is tightly coupled with Http Api, because of which the number of usecases this component can be used is limited.

Instead, the API should be open for any scenario, such as during route change/some other progress user have complete control over.

If the user wants to use this in lets say for showing & hiding during an http request, they should use library like ng2-interceptors or x-ng2-http-interceptor (I wrote this)

Once you separate this component from Http, the scope of usage for this component will go up.

Also, the page flickers like crazy in the demo, which is an indication that there is an issue with the way you are using timers in the library

@thekalinga thekalinga changed the title Remove coupling between Http api & this component Remove tight coupling with Http api Jan 25, 2017
@chihab
Copy link
Collaborator

chihab commented Aug 2, 2017

@thekalinga @aitboudad The advantage of the actual approach is that the loading bar is very easy to intergate. You just import the module and insert the component and that's it.

The idea is to be able to react to any observable start and completion and http request should keep being simple to "monitor" without a lot of boilerplate code.

There is also an issue is that http Observables are cold and request are made as soon as a subscription has been made. Normally something like

this._http.get('/app/heroes');

shouldn't perform the actual request before a call to subscribe has been made on ! Imagine that we're preparing the observable in a constructor and subscription/call is made when an event occurs.

So my idea is that the module would be configurable in order to keep the actual comfort and give the developer the ability to control the loading bar (which could be extended):

NgLoadingBarModule.forRoot({ overrideHttp: true }), // or false

If overrideHttp is true, the HTTP service is overridden and a simple call to this._http.get('/app/heroes'); would trigger the request and the loading bar (developer should be advised about the subscribe issue)

If overrideHttp is set to false, the HTTP service is not overridden, the user has to control when the loading bar should be displayed:

    startHttpRequest() {
        const request$ =
            this._http.get('/app/heroes')
                .map((response) => response.json().data);

        // Since module overrideHttp option is set to false, loading bar won't be displayed automatically

        request$.subscribe(
            (heroes) => this.heroes = heroes,
            (err) => this.loadingService.endLoading(),
            () => this.loadingService.endLoading()
        );

        // We start loading manually only when we've subscribed (or right before)
        this.loadingService.startLoading();

    }

Should work with any Observable

    startTimer() {
        const timer$ = Observable
            .interval(1000)
            .take(10);
        timer$
            .subscribe((value) => this.timer = value + 1);

        // We're sure that subscription has been made, we can start loading bar service
        this.loadingService.startLoading(timer$);
    }

I've made a PR (sorry I should probably have discuss the solution before). :)

@aitboudad
Copy link
Owner

Hi @chihab, thanks for the initial work I'll look into it this week.
what I have in mind is to split this lib into separate packages for (core, http, router):

  • @ngx-loading-bar/core
  • @ngx-loading-bar/http
  • @ngx-loading-bar/router

@chihab
Copy link
Collaborator

chihab commented Aug 2, 2017

Awesome ! especially the @ngx prefix

@aitboudad
Copy link
Owner

Published under @ngx-loading-bar:

Before:

import { NgLoadingBarModule } from 'ng-loading-bar';

@NgModule({
  imports: [
    ...,
    NgLoadingBarModule.forRoot(),
  ],
})
export class AppModule {
}

After:

import { LoadingBarHttpModule } from '@ngx-loading-bar/http';

@NgModule({
  imports: [
    ...,
    LoadingBarHttpModule,
  ],
})
export class AppModule {
}

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

No branches or pull requests

3 participants