Add start delay.

[jacknight]

This feature was previously implemented incorrectly where it left the code open to race conditions where NProgress.done() might not be called even though NProgress.start() had been, because multiple start timers could be queued up. It was also possible for a stop timer to be set, and then a start timer to also be set before the stop timer fired.

The solution is to kill both the start and stop timers whenever a router event fires, and only call NProgress.done() when NProgress.isStarted() returns true within the stopTimer. That way you can be sure NProgress.done() is called always and only when NProgress.start() has been called first.

[PabloSzx]

@apal21 any news on accepting this PR?

[apal21]

@PabloSzx I can see why we need stopDelay. But why do we need a start delay?

[jacknight]

If you don’t want the progress bar to appear for pages which load quickly.

On Sun, May 9, 2021 at 9:07 PM Apal Shah @.***> wrote:

@PabloSzx https://github.com/PabloSzx I can see why we need stopDelay. But why do we need a start delay?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/apal21/nextjs-progressbar/pull/25#issuecomment-835855171, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEN3TW2XXWNJBQIAUR75Y3TM3FMVANCNFSM4ZJDKGAA .

[apal21]

If we want our progress bar to appear for a little longer, we can always add stopDelay. I don't see why we need to see the progress bar after some time. What are we trying to achieve here? What am I missing?

[jacknight]

The progress bar appears no matter how fast the page loads. If you add a start delay, it won’t appear at all of the page loads more quickly than the start delay. The goal is to not have the progress bar appear at all for quick loading pages.

On Sun, May 9, 2021 at 10:10 PM Apal Shah @.***> wrote:

If we want our progress bar to appear for a little longer, we can always add stopDelay. I don't see why we need to see the progress bar after some time. What are we trying to achieve here? What am I missing?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/apal21/nextjs-progressbar/pull/25#issuecomment-835866110, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEN3TVYWZJ76PBOMXOOKJTTM3MZ7ANCNFSM4ZJDKGAA .

[apal21]

Got it! But adding the start delay is not an elegant solution. Let's say if you add a delay of 1 second and your page takes 2 seconds to load completely. In such cases, it'd feel like the link/button click is propagating slowly or in other words, the page is laggy. Which is not a good user experience.

[jacknight]

I don’t agree, but that’s your call.

On Sun, May 9, 2021 at 10:50 PM Apal Shah @.***> wrote:

Got it! But adding the start delay is not an elegant solution. Let's say if you add a delay of 1 second and your page takes 2 seconds to load completely. In such cases, it'd feel like the link/button click is propagating slowly or in other words, the page is laggy. Which is not a good user experience.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/apal21/nextjs-progressbar/pull/25#issuecomment-835873770, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEN3TXCWQC5O24EP7UQR3DTM3RRTANCNFSM4ZJDKGAA .

[freddieerg]

I agree with @jacknight here. I think this is an absolutely essential feature. To me, especially with the project I am working on right now, it makes absolutely no sense for 90% of my pages to show a loading indicator as all the content on those pages is already client side and will be loading in less than 300ms.

With all due respect @apal21 surely this is just a matter of opinion and should be a feature that the developer is able to decide for themselves whether or not they want to use?

[apal21]

@freddieerg I'm not against having this feature but the way we're trying to implement is not elegant as I mentioned in the comment above. If you have any better ideas we can implement them.

[freddieerg]

What is it exactly you don't thing is elegant sorry? If your issue is just "delay of 1 second and your page takes 2 seconds to load" I would argue many developers would say showing the loading indicator on every page that's taking less than 300ms to load is even less elegant.

But as I said before, in my opinion this should be left to the developer to decide on their personal preference. The PR looks like a sound implementation unless I'm missing something and would provide exactly the functionality that I and many others are looking for. I'd much prefer this was merged and published than having to add the package directly from @jacknight's fork.

[freddieerg]

Actually don't worry if you don't want to implement it. @jacknight has published his fork to NPM with the delayMs option.

[apal21]

Awesome!