ISSUE-90: Better handling of synchronous (sequential) requests

[gnom7]

• added support for extraDuration feature
• added test for extraDuration feature
• extraDuration should delay spinner hiding at specified time (in millis), this should help to avoid spinner blinking on sequential requests (when crafting of one request depends on data from previous request, so that between those request may arrive ~5ms gap)

[coveralls]

Pull Request Test Coverage Report for Build 500

• 18 of 18 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 100.0%

---

Totals
Change from base Build 491:	0.0%
Covered Lines:	181
Relevant Lines:	181

---

💛 - Coveralls

[gnom7]

Not sure why one test failed, I run all tests locally and all passes. Tried same node and browser version as on CI, no luck. Should dig into this more.

[mpalourdio]

Awesome ! I need to give this a shot ! I also need to fix things on my side before, before I'd like we handle these 2 feature requests in a cleaner way, by introducing switchMap, so we can complete/bypass debounce && delayWhen pipes. As I am committing to master sometimes, don't forget to rebase your branch onto from time to time.

FWIW, I need to take decisions on how to mix different options too. Tests should include all mixed cases if we allow mixing, or we need to throw exceptions.

Once again, thanks a lot. Hope I can give this a look next week, that's awesome !

[gnom7]

I realized that test failure is not caused by this PR.
How do you intent to use switchMap? It is not clear to me.

[mpalourdio]

Yes, master is not stable. The last beta release has revealed an old and dirty bug. Then I realized the way debounce and delay are handled is bad, and should be gracefully handled before subscription, not at runtime. That's why I think using switchMap in the pipe.

It's hard to explain at the moment, and working code will be hopefully easier to read. I need some time to fix it. I want to get rid of all those 'if' before timers etc. Current implementation has shown its limit.

[gnom7]

introducing switchMap, so we can complete/bypass debounce && delayWhen pipes

updated

get rid of all those 'if' before timers

updated

tried to use partition instead filter, but then CI fails for some reason

[mpalourdio]

Wow, that's definitely awesome ! Thanks a lot !

Would you mind rebasing your branch onto master ? It introduces a failing test-case because of a bug that I've caught after releasing beta yesterday ?

[gnom7]

I have merged, but test still fails. Have you any guesses why test fails on CI? Are you able to reproduce it locally?

[mpalourdio]

Yes tests fail locally too, that's expected. You have changed the wrong test by introducing flush(). Please revert. Existing tests should remain untouched as much as possible.

The failing test is this one :

ng-http-loader/src/test/components/ng-http-loader.component.spec.ts

Line 500 in 4dc80cd

it('should correctly handle the minimum spinner duration for multiple HTTP requests ran one after the others', fakeAsync(inject(

[gnom7]

You have changed the wrong test

I see, sorry :)
Sadly I'm not able to reproduce test failure locally, so I push changes to remote to trigger CI build

[mpalourdio]

That's weird. I jun run 'yarn test' or run tests directly from IntelliJ.

We'll cleanup the commit history afterwards,not a big deal.

[mpalourdio]

I am going to cleanup/squash and push force to your branch if this is OK for you, so I can add commits and have a clean base. Just let me know if this is OK

[gnom7]

I am going to cleanup and push force to your branch if this is OK for you, so I can add commits and have a clean base. Just let me know if this is OK

yes, sure, although I already discarded those commits to fix test, but for further cleanup, if you see some, it is ok

[gnom7]

could you clarify please what does h mean?

[mpalourdio]

Awesome ! But another test is failing now :)

[mpalourdio]

The test in only failing on chrome, not firefox oO

[mpalourdio]

The test is fine when ran standalone, but not during the full test suite. Something with dates/ race concurrency I suppose.

[gnom7]

I suppose now it is fixed :)
I believe that in test, in case js code run fast, after component is created and visibleUntil initialized with Date.now(), new request gets triggered immediately, and strong this.visibleUntil < now comparison fails, so we need to use either <= or initialize visibleUntil with Date.now() - 1, what would you say?

[mpalourdio]

hey, that's awesome ! You rock :) Merging into master now, so I can add some tests and documentation.

Once again, a big thank you \o/