Skip to content

blockAllRequestsInProgress option for BlockUIHttpModule does not block 2nd HTTP request for sequential requests #126

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

Closed
joboyx opened this issue Apr 21, 2020 · 17 comments

Comments

@joboyx
Copy link

joboyx commented Apr 21, 2020

INFO:

  • affected version: 2.1.1 and 2.1.8

From the parameter name blockAllRequestsInProgress, user expects that this will block all HTTP calls regardless if there are called in parallel or sequentially.

However in our app, there are instances where in if there are 2 consecutive HTTP requests (usually blocked by await) the first request gets blocked, but the 2nd request is not blocked.

As workaround what I did was to set blockAllRequestsInProgress to false.

Which is misleading. And may cause issues for use cases where transactions should not be called repeatedly in parallel (debounce).

Sample code:

    BlockUIModule.forRoot({
      delayStart: 0,
      delayStop: 100,
      message: 'loading...',
      template: BlockUiLoaderComponent
    }),
    BlockUIHttpModule.forRoot({
      blockAllRequestsInProgress: true,
      requestFilters: [urlFilter]
    }),

There's also a question in StackOverflow which I answered with my workaround.

https://stackoverflow.com/questions/58714797/ng-block-ui-not-working-with-angular-7-concatmap/61338294#61338294

Also even if I upgrade to 2.1.8, issue still exists.

Thanks!

@kuuurt13
Copy link
Owner

@joboyx thanks for all the info! Have you tried updating to the 3.0.0-beta to see if this fixes the issue (npm i ng-block-ui@next)?

@kuuurt13
Copy link
Owner

@joboyx I just wanted to follow up my last comment with these 2 stackblitz projects. I created 2 different scenarios, one with parallel requests and the other with nested requests. I ended up creating these using [email protected] and I can't seem to reproduce the issue. If you could fork these to reproduce your issue, then I might be able to help.

https://stackblitz.com/edit/github-http-nested-6tpjzq?file=src%2Fapp%2Fapp.component.ts
https://stackblitz.com/edit/github-http-nested-delay?file=src%2Fapp%2Fapp.component.ts

@joboyx
Copy link
Author

joboyx commented Apr 22, 2020

Thanks for those test projects, didn't know we can do that. :)

Here, I forked your stackblitz project and was able to reproduce the issue:

https://stackblitz.com/edit/github-http-nested-kgehid

Issue occurs with Promise with await/async.

@kuuurt13
Copy link
Owner

@joboyx thanks for the example. So I forked your stackblitz and updated ng-block-ui to use the 3.0.0-beta and it fixes the issue https://stackblitz.com/edit/ng-block-ui-issue-126?file=src/app/app.component.ts

Over the lifespan of the library, I have had a few PR come in that fixed someones specific issue but broke other edge cases so in 3.0.0 I have completely rewritten the block queuing to encompass all edge cases. Can you try updating your app to use the new beta and let me know if it fixes the issue? (npm i ng-block-ui@next)

@joboyx
Copy link
Author

joboyx commented Apr 29, 2020

Thanks upgrading to 3.X fixed my issue. Here's the update stackblitz project with all of my use cases working with 8.X -- https://stackblitz.com/edit/ng-block-ui-issue-126-axsq2v

Just want to note that to make it work both for Promise and Observers I need to set blockAllRequestsInProgress to true. Setting this to false will have the same issue with 2.1.8.


However, if I apply the changes for 3.X to our code base some instances are now broken. Like on this page. The block-ui did not unblock totally even after all requests are done. In this scenario 2 requests are subscribe and 1 request is a promise without await.

But I can't seem to replicate this issue in the stackblitz project I provided above (using 3.X with blockAllRequestsInprogress=true. Looks like requests in stackblitz are not executed in parallel??? Not sure.

image

So given this I can't still use this as a solution, it is more broken that it actually is in 2.1.8.

As workaround I'm converting our nested and parallel Observable.subscribe() to promises and this make ng-block-ui work properly with blockAllRequestsInprogress=true and 2.1.8.

@kuuurt13
Copy link
Owner

Thanks for the additonal info. Let me keep looking into this, thanks.

@kuuurt13
Copy link
Owner

kuuurt13 commented May 8, 2020

@joboyx so after doing a bunch of testing, I think I have fixed the issue you are seeing. I have released a new version (3.0.0-beta.10) with these changes. I have also forked your stackblitz and upgraded to the latest beta and it looks like everything is working, see here.

Take a look and let me know if this fixes your issue.

@joboyx
Copy link
Author

joboyx commented May 18, 2020

@kuuurt13, I've tested 3.0.0-beta.10 in our code and it fixed the specific scenario I mentioned here -- #126 (comment) -- which is my blocker to update to this version in our code. I've integrated this version in our code and pushed to testing.

Thanks for the effort on fixing this! Really appreciate it. It's my first time to have an interaction like this with a maintainer. :)

I think you can close this now. Thanks.

@joboyx
Copy link
Author

joboyx commented May 18, 2020

Just one more thing. This parameter -- blockAllRequestsInProgress is currently set to false in our app.module.ts. It would be good to clarify what's the purpose of this flag in the documentation? Take not that changing this value also changed the behavior of the bug.

@kuuurt13
Copy link
Owner

@joboyx I'm glad it fixed your issue. Thanks for providing so many details for your issue, it helped a lot!

As for blockAllRequestsInProgress, there already are docs for this that are linked to from the main docs page, see here. The reason this setting exists was because it was added to allow for backwards compatibility in a previous version. Most people prefer to block all pending request but this setting was added so it was opt in and wouldn't change the behavior for people currently using the http module. This will now be defaulted to true in 3.0.0 and the docs have been updated for 3.0.0, see here

@joboyx
Copy link
Author

joboyx commented May 20, 2020

Hmm okay. But just want you to note that I've still encountered a scenario where some requests are still ongoing but block-ui already unblocked if blockAllRequestsInProgress is set to true. And setting it to false resulted to the behavior where UI will block until all reqeusts are finished (which is the opposite of the config name).

Sorry, but I won't be able to give you the specific scenario. But that's my current config as stated above. Not expecting any action from you for this. Just an FYI.

Thanks again.

@kuuurt13
Copy link
Owner

@joboyx if you can pinpoint and reproduce the issue where this happening feel free to share and I can take a look. As for that setting, from my testing it seems to be working as designed. I created a comprehensive example here that shows how it is working. Feel free to change the setting and see how it impacts the blocking.

@joboyx
Copy link
Author

joboyx commented May 28, 2020

Found another issue in v3.0.0-beta.10. requestFilters: [urlFilter] is not taking effect. All URLs are still being blocked.

export function urlFilter(req: HttpRequest<any>): boolean {
  return new RegExp('200').test(req.url);
}

@NgModule({
  entryComponents: [BlockUiComponent],
  declarations: [AppComponent, BlockUiComponent],
  imports: [
    BrowserModule,
    HttpClientModule,
    BlockUIModule.forRoot({
      delayStart: 0,
      delayStop: 100,
      message: "loading...",
      template: BlockUiComponent,
    }),
    BlockUIHttpModule.forRoot({
      blockAllRequestsInProgress: false,
      requestFilters: [urlFilter]
    })
  ],
  providers: [],
  bootstrap: [AppComponent]
})
export class AppModule {}

See this stackblitz project -- https://stackblitz.com/edit/ng-block-ui-issue-126-request-filter

Take note that when running the project in stackblitz -- the issue does NOT happen, as expected, no block-ui is happening.

But if you download the project and run locally, all requests are still being blocked even if there's requestsFilters specified. I think there's some rate limiting being done by stackblitz to prevent concurrent HTTP calls causing for this issue to not happen in stackblitz but happens locally, and also in our own project code base.

@kuuurt13
Copy link
Owner

If it is only happening locally then it might be an issue with Angular's compiler since I think stackblitz serves Angular apps differently. I looked real quick locally and it looks like requestFilters is not even coming through via the settings in your example so let me take a look and see why Angular removes it when compiling.

@kuuurt13
Copy link
Owner

@joboyx it looks like there was an issue with how Angular was handling values passed to BlockUIHttpModule.forRoot(). It looks like during compilation it is was handling useValue differently. I'm guessing StackBlitz must do something different than when you run it locally. Anyway, I have made an update and it should work the same no matter if ran in StackBlitz or locally. Please install beta.11 and let me know if it is working for you locally now.

@joboyx
Copy link
Author

joboyx commented May 29, 2020

3.0.0-beta.11 fixed the issue on requestFilters option! Thanks for the quick turnaround on this.

Btw, the issue I mentioned with blockAllRequestsInProgress. With the latest version, I need to set the value of blockAllRequestsInProgress to true now. To prevent the blinking of blockUi component between requests. So this option now behaves as expected. Also understand that this is now the default value.

@kuuurt13
Copy link
Owner

kuuurt13 commented Jun 1, 2020

Great, good to hear! Going to close this then. Thanks again for providing all these details/examples to help me reproduce the issue.

@kuuurt13 kuuurt13 closed this as completed Jun 1, 2020
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

2 participants