-
Notifications
You must be signed in to change notification settings - Fork 241
feat: allow $loading indicator throttling #247
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
Conversation
Codecov Report
Merging #247 into dev will not change coverage.
The diff coverage isn/a.
@@ 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.
5b505c5 to
777b63e
Compare
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 Report
Merging #247 into dev will not change coverage.
The diff coverage isn/a.
@@ 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.
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
There was a problem hiding this comment.
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?
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.
(ping @simllll)
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
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.
any news?
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