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

feat: allow $loading indicator throttling #247

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

simllll
Copy link
Contributor

@simllll simllll commented May 13, 2019

Problem:
Right now the loading indicator is shown even for very short request. Which results in a "flashing" that looks more like a bug than a feature ;)

Reason & Solution:
when using .set on $loading it shows immediately the loading indicator. (see nuxt-loading.vue) The indicator itself has a throttle property though (which is only checked on .start()). As long as we only have one request, there is no additional benefit of using .set directly, therefore we can rely on the throttle parameter by using start() instead.

Different approach:
Another approach would be implementing our own "throttle" method which sets a timer on "onRequest" when it's not set) or currentRequests === 0), and removes the timer again onResponse when currentRequests <= 0.
But then we need another config option for the throttling param, or can we access the one from nuxt.config's loading property somehow?

Regards
Simon

@codecov
Copy link

codecov bot commented May 13, 2019

Codecov Report

Merging #247 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff         @@
##            dev   #247   +/-   ##
===================================
  Coverage   100%   100%           
===================================
  Files         1      1           
  Lines        27     27           
  Branches     12     12           
===================================
  Hits         27     27

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f744666...777b63e. Read the comment docs.

@simllll simllll force-pushed the patch-1 branch 2 times, most recently from 5b505c5 to 777b63e Compare May 13, 2019 15:11
author Simon Tretter <simllll@users.noreply.github.com> 1557759137 +0200
committer simon <s.tretter@gmail.com> 1557760081 +0200

allows $loading indicator throttling

Problem:
Right now the loading indicator is shown even for very short request. Which results in a "flashing" that looks more like a bug than a feature ;)

Reason & Solution:
when using .set on $loading it shows immediately the loading indicator. (see nuxt-loading.vue) The indicator itself has a throttle property though (which is only checked on .start()). As long as we only have one request, there is no additional benefit of using .set directly, therefore we can rely on the throttle parameter by using start() instead.

Different approach:
Another approach would be implementing our own "throttle" method which sets a timer on "onRequest" when it's not set) or currentRequests === 0), and removes the timer again onResponse when currentRequests <= 0.
But then we need another config option for the throttling param, or can we access the one from nuxt.config's loading property somehow?

Regards
Simon
@codecov
Copy link

codecov bot commented May 13, 2019

Codecov Report

Merging #247 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff         @@
##            dev   #247   +/-   ##
===================================
  Coverage   100%   100%           
===================================
  Files         1      1           
  Lines        27     27           
  Branches     12     12           
===================================
  Hits         27     27

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f744666...5653c6c. Read the comment docs.

@simllll
Copy link
Contributor Author

simllll commented May 15, 2019

I've seen there is also another PR hanging arround regarding this issue. I've solved this for me now by:
1.) disabling progress updates globally via nuxt.config:

	axios: {
		progress: false, 
       }

2.) using my own axios-enrichment plugin, which I have in place anyway where I added follwoing code:
plugins/axios-progress.ts:

import axios from 'axios';

export default ({ app, $axios, store, res }) => {
	const noopLoading = {
		finish: () => {},
		start: () => {},
		fail: () => {},
		set: () => {}
	};

	const $loading = () =>
		process.client && window.$nuxt && window.$nuxt.$loading && window.$nuxt.$loading.set
			? window.$nuxt.$loading
			: noopLoading;

	let requestsRunning = 0;
	
	$axios.onResponse(response => {
		requestsRunning -= 1;
		if (requestsRunning <= 0) {
			requestsRunning = 0;
			$loading().finish();
		}
	});

	$axios.onRequest(config => {
		requestsRunning += 1;
		if (requestsRunning === 1) {
			$loading().start();
		}
	});

	$axios.onError(error => {
		requestsRunning -= 1;
		if (!axios.isCancel(error)) $loading().fail();

		if (requestsRunning <= 0) {
			requestsRunning = 0;
			$loading().finish();
		}
	});
};

dont forget to add this in nuxt.config as plugin:
plugins: ['~/plugins/axios-progress']

hope it's helpful.

regards
Simon

@pi0 pi0 changed the title allows $loading indicator throttling feat: allow $loading indicator throttling Jun 5, 2019
@@ -139,8 +142,10 @@ const setupProgress = (axios, ctx) => {
if (!currentRequests) {
return
}
const progress = ((e.loaded * 100) / (e.total * currentRequests))
$loading().set(Math.min(100, progress))
if (currentRequests > 1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this condition, we don't precisely show progress with single requests (but random jumps). Is it desired?

@pi0
Copy link
Member

pi0 commented Jun 5, 2019

Thanks, @simllll for this PR and sorry about late review/answer. I just left a minor comment about this change. I think what you have proposed in your second comment is more reasonable (only call to start() on the first request) but always update progress.

@manniL
Copy link
Member

manniL commented Jul 25, 2019

(ping @simllll)

@simllll
Copy link
Contributor Author

simllll commented Jul 25, 2019

hi @manniL,
my last comment is more like a different approach, by just disabling the module's indicator and set up my own. Would you like me to adapt these changes to the module itself?

I think what you have proposed in your second comment is more reasonable (only call to start() on the first request) but always update progress.

The problem with that is, as soon as we set the progress the bar appears, therefore this would need some special handling which I'm unsure if this is really necessary.
The progress of one request is not computed anyway, as onResponse is only called at the end of a request. Therefore a real benefit of the "progress" is only existing if there are a lot of parallel requests running. (Or we find a way to update the progress bar for longer running requests more frequently (e.g. during uploads or file downloads...))

Regards
Simon

@hecktarzuli
Copy link
Contributor

I would really think there would be a larger layer of abstraction between the loading component and it's integration with plugins. I would assume there would be a way for plugins to simply say, "hey nuxt, I'm loading 3 things.... now I'm loading 2 things.... now 1 etc..." and nuxt's loading component would figure out how to render that abstraction over time. I really don't think plugins should be talking directly to the loading component.

@pi0 pi0 added the pending label Dec 17, 2019
@gaodeng
Copy link
Contributor

gaodeng commented May 28, 2020

any news?

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

Successfully merging this pull request may close these issues.

None yet

5 participants