⚡ Don't re-render the plugin repository if notmodified #4132
Conversation
github-actions
bot
added
the
approved
For reference, the additional processing done here adds up to less than 100ms for me. |
| .done(function (data, status, xhr) { | ||
| // Don't update if cached - requires ifModified: true to pass through | ||
| // the 304 status, otherwise it fakes it and produces 200 all the time. | ||
| if (xhr.status === 304) return; |
There was a problem hiding this comment.
Thought of the day - if this case is hit, does the promise ever get resolved/rejected? Might need to look at that. Just a thought that crossed my head...
There was a problem hiding this comment.
From my understanding it should not interfere. The done callback has no way to interfere with the promise processing, it's just a callback and returning from it won't affect any other callbacks.
First of all, definitely makes sense. Would it be a good idea to do that on the client lib layer instead of in the viewmodels? |
The only thing about that is that it would technically be a breaking change - since Off the top of my head, it was printer profiles, access control, backup and logs that refreshed themselves like this. |
What does this PR do and why is it necessary?
Don't re-render the plugin repository if the server responds with
notmodified. This shaves ~500ms off the total processing time, making opening the settings dialog lightning fast ⚡.How was it tested? How can it be tested by the reviewer?
Using the performance profiler, first I identified the issue, then opened and closes the settings dialogs several times, to make sure this change was effective.
Any background context you want to provide?
General push to improve UI performance, started with the animations, onto the data processing...
What are the relevant tickets if any?
Closes #4131
Screenshots (if appropriate)
Look, barely a dropped frame...
Comparison to before this change:
Further notes
Ok, now another question. I noticed some of the other endpoints that are processed repeatedly like this, and wondered if it would make sense to implement the same thing?
It has to be on a per-request basis, since adding
ifModified: trueto all request options breaks handlers that are not expecting an empty response.