-
Notifications
You must be signed in to change notification settings - Fork 285
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(storage-browser): add useProcessTasks #5905
feat(storage-browser): add useProcessTasks #5905
Conversation
|
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.
Looks good to me. Only have 1 question.
branches: 81, | ||
functions: 83, | ||
lines: 91, | ||
statements: 90, |
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.
👍
| 'FAILED' | ||
| 'COMPLETE' | ||
| 'QUEUED' | ||
| 'PENDING'; |
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.
What's the difference between PENDING
and QUEUED
?
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.
PENDING
-> in progress/processingQUEUED
-> initial status, "not started"
let count = 0; | ||
while (count < concurrency) { | ||
_processTasks(input); | ||
count++; | ||
} |
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.
The predicate here needs to be updated to compare the size of the inflight tasks to prevent spawning additional concurrent tasks on calls to processTasks
while processing is active. Noting to be addressed in a follow up
isCancelable?: boolean; | ||
} | ||
|
||
export interface Task<T = any> { |
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.
How does this any
keep from leaking or do we have to keep it wide in order to allow any type?
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.
Yep, needs to be wide for consumers to define the type of item
. Providing any
as the default generic allows for using Task
in utils without specifying a type
cancel: undefined | (() => void); | ||
item: T; | ||
key: string; | ||
message: string | undefined; |
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.
can this be message?: string
?
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.
Output values should not be optional unless defined by the consumer
}) | ||
.finally(() => { | ||
inflight.current.delete(key); | ||
_processTasks(input); |
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.
can we end up in an infinite loop here?
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.
Not unless i'm missing something, do you have a specific point of concern?
Description of changes
Add
useProcessTasks
utility hook for usage in batch action default and custom views, e.g.Upload
,Copy
, etcIssue #, if available
NA
Description of how you validated changes
Manual testing, unit tests
Checklist
yarn test
passes and tests are updated/addeddocs
,e2e
,examples
, or other private packages.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.