Skip to content
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

Merged
merged 22 commits into from
Aug 8, 2024
Merged

RJS-2867: Add useProgress and tests #6804

merged 22 commits into from
Aug 8, 2024

Conversation

gagik
Copy link
Contributor

@gagik gagik commented Jul 22, 2024

What, How & Why?

This closes #6797

@realm/devdocs

☑️ ToDos

  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 📝 Update COMPATIBILITY.md
  • 🚦 Tests
  • 📦 Updated internal package version in consuming package.jsons (if updating internal packages)
  • 📱 Check the React Native/other sample apps work if necessary
  • 💥 Breaking label has been applied or is not necessary
  • 🔔 Mention @realm/devdocs if documentation changes are needed

@cla-bot cla-bot bot added the cla: yes label Jul 22, 2024
@gagik gagik marked this pull request as draft July 22, 2024 14:48
@gagik gagik marked this pull request as ready for review July 23, 2024 08:53
@gagik gagik changed the title Add useProgress and tests RJS-2867: Add useProgress and tests Jul 23, 2024
@gagik gagik requested a review from kraenhansen July 23, 2024 08:59
@gagik gagik requested a review from kneth August 7, 2024 10:47
Copy link
Contributor

@kneth kneth left a 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.

packages/realm-react/CHANGELOG.md Outdated Show resolved Hide resolved
Base automatically changed from gagik/realm-fallback-process to main August 8, 2024 09:21
@gagik gagik merged commit f4963cc into main Aug 8, 2024
8 checks passed
@gagik gagik deleted the gagik/add-use-progress-hook branch August 8, 2024 09:52
@@ -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";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong 🤔

Copy link
Contributor Author

@gagik gagik Aug 12, 2024

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]) => {
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

@kraenhansen kraenhansen Aug 12, 2024

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][]
Copy link
Member

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 🤞

Copy link
Contributor Author

@gagik gagik Aug 12, 2024

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.

Copy link
Member

Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add useProgress to @realm/react
3 participants