-
Notifications
You must be signed in to change notification settings - Fork 585
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
RJS-2867: Add useProgress and tests #6804
Conversation
…-js into gagik/add-use-progress-hook
…-js into gagik/add-use-progress-hook
…-js into gagik/add-use-progress-hook
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.
My comment shouldn't block you but please update the changelog entry.
@@ -19,6 +19,7 @@ | |||
import { act } from "@testing-library/react-native"; | |||
import { EstimateProgressNotificationCallback, ProgressRealmPromise, Realm } from "realm"; | |||
import { sleep } from "../helpers"; | |||
import { SyncSession } from "../../../realm/dist/public-types/internal"; |
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.
This seems wrong 🤔
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.
Huh weird, not sure how I didn't get much errors or anything; the sleep above is also referring to a level above it should. created PR for these: #6833
[ProgressMode.ForCurrentlyOutstandingWork, ProgressDirection.Download], | ||
[ProgressMode.ForCurrentlyOutstandingWork, ProgressDirection.Upload], | ||
] as [ProgressMode, ProgressDirection][] | ||
).forEach(async ([mode, direction]) => { |
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 a fan of forEach
as it's not obvious that the callback is called synchronously.
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.
actually this function itself doesn't have to be defined as async, would that be okay then?
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'd rather that this was a for-of loop. I personally never use the forEach
for this reason (that no guarantees are made on how the callback is invoked and this code expects it to be invoked synchronously).
[ProgressMode.ReportIndefinitely, ProgressDirection.Upload], | ||
[ProgressMode.ForCurrentlyOutstandingWork, ProgressDirection.Download], | ||
[ProgressMode.ForCurrentlyOutstandingWork, ProgressDirection.Upload], | ||
] as [ProgressMode, ProgressDirection][] |
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'm sure this could be written without the type assertion 🤞
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.
unfortunately not, it thinks of itself as an array of (ProgressMode | ProgressDirection)
, I wish typescript was a little smarter.
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.
Oh cool! I remember using as const
before but dind't think of this in that scenario
What, How & Why?
This closes #6797
@realm/devdocs
☑️ ToDos
Compatibility
label is updated or copied from previous entryCOMPATIBILITY.md
package.json
s (if updating internal packages)Breaking
label has been applied or is not necessary@realm/devdocs
if documentation changes are needed