-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Reduced number of requests from client to server #7446
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #7446 +/- ##
===========================================
+ Coverage 83.44% 83.54% +0.10%
===========================================
Files 374 374
Lines 39753 39753
Branches 3729 3731 +2
===========================================
+ Hits 33170 33213 +43
+ Misses 6583 6540 -43
|
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.
It looks PR lacks changelog and package.json updates
@@ -309,11 +309,11 @@ const tasksDummyLabelsData = { | |||
attributes: [], | |||
}], | |||
100: [{ | |||
id: 1, |
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.
Why do we need such change?
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.
We have other labels with id: 1 in mock data, and as I discovered, in one of tests we remove label and it removes all labels with the same id from everywhere
// | ||
// SPDX-License-Identifier: MIT | ||
|
||
import { ActionUnion, createAction, ThunkAction } from 'utils/redux'; | ||
import { RegisterData } from 'components/register-page/register-form'; | ||
import { getCore } from 'cvat-core-wrapper'; | ||
import { getCore, User } from 'cvat-core-wrapper'; |
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.
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.
#7431 is not merged
When we merge this one, these changes will disappear from another PR.
So, what is the problem?
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.
I added it here because now in UserSelector component I need current user id. And, there were not types before
} | ||
|
||
const key = `${userID}_${organizationSLUG || ''}`; | ||
const RELOAD_INITIAL_USERS_AFTER_MS = 300000; |
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.
Isnt this number too large?
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.
I do not see any problems with 5 minutes. If user can't see somebody's name, typical behavior is to start entering this name. This memoization will by bypassed.
Motivation and context
Memoize initial users to not request list for each UserSelector, there are plenty of these selectors, especially on task page with a list of jobs
Do not make extra save annotation request with empty body
When moving a task to a project, patch labels requests are send one by one, not in parallel
Do not duplicate exceptions events in dev environment (see. The fake event trick for rethrowing errors in DEV fires unexpected global error handlers and makes testing harder facebook/react#10474)
Do not create excessive requests when lots of exceptions happen.
But when UI failed, logs are sent immediately
How has this been tested?
Checklist
develop
branch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.