ng-http-loader icon indicating copy to clipboard operation
ng-http-loader copied to clipboard

Usage of dynamic filteredUrlPatterns with async pipe

Open giovanni-bertoncelli opened this issue 1 year ago • 6 comments

Expected behavior

When I'm using [filteredUrlPatterns] with an async pipe I expect that the component filters out the HTTP calls according to the current subscription value.

Actual behavior

The component filters only for the first value of the subscription

Steps to reproduce

  • Use [filteredUrlPatterns]="Observable | async"
  • Change next value of Observable on different routes, the filters different from the first does not apply

ng-http-loader version

14.0.0, Angular 16

It does not work as expected on Internet explorer or Safari

Nothing works as expected on Internet explorer or Safari.

giovanni-bertoncelli avatar Mar 14 '24 09:03 giovanni-bertoncelli

Indeed, that could be cool, but needs a bit of refactor. Marking as enhancement. Feel free to PR.

mpalourdio avatar Mar 17 '24 08:03 mpalourdio

Thank you @mpalourdio. I looked a bit inside the code and it's probably an issue with every possible filter @Input() inside ng-http-loader component. Is it maybe a good idea to wrap all the possible filters inside a Subject/BehaviourSubject that is updated each time the related component changes @Input values and it's piped when intercepting a request with something like withLatestFrom?

The pending-requests-interceptor.service.ts should look something like this:

export interface InterceptorFilters {
  urlPatterns?: RegExp[],
  methods?: string[],
  headers?: string[]
}

export class PendingRequestsInterceptor implements HttpInterceptor {
  // ... other stuff
  private _filters$ = new BehaviourSubject<InterceptorFilters>({}) 
  
  // setter called by component (same for the other filters)
  set filterUrlPatterns(newFilter: RegExp[]) {
      this._filters$.next({
           ...this._filters$.value,
           urlPatterns: newFilter
      })
  }

intercept(req: HttpRequest<any>, next: HttpHandler): Observable<HttpEvent<any>> {
    return next
       .pipe(
        withLatestFrom(this._filters$),
        tap(([_, filters]) => {
          // evaluate filter with this.shouldBypass method
          // and trigger or not _pendingRequestsStatus$.next(true)
        })
     )
      .handle(req)
      // .. other stuff
  }
}

The http-loader component should only call the setter each time the related @input changes. What do you think?

giovanni-bertoncelli avatar Mar 18 '24 09:03 giovanni-bertoncelli

Looks sexy, reactive && clean but my main concern is backward compatibility. We need to have a way to not break existing apps that use this lib. As is, BC is broken for sure.

I have never liked this part. It makes the service stateful, and it breaks encapsulation in many ways. But, i don't want to break anything for sure.

Any idea?

mpalourdio avatar Mar 21 '24 19:03 mpalourdio

Thanks @mpalourdio, do you need retrocompatibility on the pending-requests-interceptor.service.ts service or the ng-http-loader component? Because for the second one there would be no breaking changes, for the first one (if its API should be considered public) we can provide some getters for the old filters members:

get filterUrlPatterns() { // we can use the old names if necessary
   return  this._filters$.value.urlPatterns;
}

giovanni-bertoncelli avatar Mar 22 '24 08:03 giovanni-bertoncelli

I am an old school boy, I tend to think that every public API should be BC

mpalourdio avatar Mar 22 '24 09:03 mpalourdio

Sure, no problem.. I may try a PR in the next weeks

giovanni-bertoncelli avatar Mar 22 '24 09:03 giovanni-bertoncelli