RJS-2636: Add progress notifications and tests #6743

gagik · 2024-06-21T07:48:23Z

What, How & Why?

Adds progress notifications for flexible sync and tests for it.

realm.syncSession?.addProgressNotification(
  ProgressDirection.Upload,
  ProgressMode.ReportIndefinitely,
  (estimate) => console.log(`progress: ${estimate}/1.0`)
);

This closes #6256

@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~~

gagik · 2024-06-21T14:20:41Z

integration-tests/tests/src/tests/sync/sync-session.ts

@@ -49,6 +50,7 @@ const PersonForSyncSchema: Realm.ObjectSchema = {
  },
 };

+// TODO: Fix this.


This is unrelated to this PR, I can create a follow-up issue but waitForConnectionState seems to be a complete noop right now, may mean some tests either don't need it or are meaningless.

integration-tests/tests/src/tests/sync/flexible.ts

gagik · 2024-06-24T11:08:44Z

integration-tests/tests/src/tests/sync/sync-session.ts

+      const progressCallback = spy();
+      await Realm.open(config).progress(progressCallback);
+
+      expect(progressCallback.called).is.true;


Not going to spend much time refactoring the other tests since they related to the deprecated method, just thought this was a nice opportunity for sinon (also just timeout).

Also, there's a sinon-chai integration and I will switch to that soon, maybe in a different PR if there's not enough time for now

gagik · 2024-06-26T09:11:53Z

I am still battling some CI issues but I figure it may be worth requesting some reviews for now.

gagik · 2024-06-26T10:48:07Z

integration-tests/tests/package.json

@@ -84,6 +84,8 @@
    "@types/chai-as-promised": "^7.1.5",
    "@types/jsrsasign": "^10.5.4",
    "@types/mocha": "^10.0.0",
+    "@types/sinon": "^9.0.5",
+    "assert": "^2.1.0",


This polyfill seems necessary for electron...

I'd like more context on this change.

It seemed necessary for sinon but I had since then changed the version to something which has more compatibility so it may not be needed anymore

gagik · 2024-06-26T12:49:48Z

packages/realm/src/ProgressRealmPromise.ts

@@ -167,18 +175,31 @@ export class ProgressRealmPromise implements Promise<Realm> {
   */
  progress(callback: ProgressNotificationCallback): this {
    this.listeners.add(callback);
+    if (callback.length === 1) {
+      const estimateCallback = callback as DynamicProgressNotificationCallback;
+      estimateCallback(1.0);


since this behavior is sort of expected from previous implementation, I figured this was best equivalent.

gagik · 2024-06-26T14:10:55Z

Okay, I am 99% certain that last failing tests is a result of GHA caching, and I tried to delete the cache manually many times for this branch/PR but seems to keep using it, @kraenhansen may have a better idea of what is going on here... but will have to leave it at that since this now 62 commits deep into making it fully green 😄 At least it does not seem flaky and is somewhat reasonable in its underlying implementation.

Will leave it at this point since there's no chance to have a proper reviews but feel free to continue from here to get it merged. I'd say it is pretty complete and although tests are far from ideal, there is also no clear behavior expectations yet and much of it is just up to core.

Should be good to merge beyond any review comments you can have. Feel free to get in touch if needed in next few days.

kneth

LGTM

elle-j

Comments from a first pass here, looks really nice 🙂

elle-j · 2024-06-27T11:21:44Z

packages/realm/src/ProgressRealmPromise.ts

+  /** @internal */
+  private token = 0;


Is 0 being used as an "unset" indicator here? It could possibly make it clearer if we used e.g. -1 (as the token in Core uint) or null.

We should also have the number type be binding.Int64 rather than number. (See e.g. this)

Could we add a short sentence to the internal docs here mentioning that it's used for unregistering the notifier?

elle-j · 2024-06-27T11:31:12Z

packages/realm/src/ProgressRealmPromise.ts

-        // TODO: Consider storing the token returned here to unregister when the task gets cancelled,
-        // if for some reason, that doesn't happen internally
-        this.task.registerDownloadProgressNotifier(this.emitProgress);
+        if (this.listeners.size > 0) {


Isn't it possible for a listener to be added (via progress()) after this code has been executed? If so, then I think this guard should probably be removed. (Invoking the listeners is still guarded by it's size here.)

elle-j · 2024-06-27T11:37:28Z

packages/realm/src/ProgressRealmPromise.ts

+      if (this.token !== 0) {
+        this.task?.unregisterDownloadProgressNotifier(this.token);
+      }


Should we reset the token after the Core call here as well?

elle-j · 2024-06-27T11:45:41Z

packages/realm/src/ProgressRealmPromise.ts

@@ -153,6 +160,7 @@ export class ProgressRealmPromise implements Promise<Realm> {
  cancel(): void {
    this.cancelAndResetTask();
    this.timeoutPromise?.cancel();
+    this.task?.unregisterDownloadProgressNotifier(this.token);


We could guard for the "unset" token value here.

elle-j · 2024-06-27T11:53:35Z

packages/realm/src/ProgressRealmPromise.ts

@@ -167,18 +175,31 @@ export class ProgressRealmPromise implements Promise<Realm> {
   */
  progress(callback: ProgressNotificationCallback): this {


The API docs for this function needs an update 🙂

elle-j · 2024-06-27T12:09:55Z

packages/realm/src/app-services/SyncSession.ts

@@ -93,6 +109,21 @@ function toBindingDirection(direction: ProgressDirection) {
  }
 }

+function isEstimateProgressNotification(cb: ProgressNotificationCallback): cb is DynamicProgressNotificationCallback {


I believe a Callback postfix is missing from the name.

Could we use the fully spelled out name callback for increased consistency?

agree with the callback, these bits of code are a mix of mine and largely @kneth so he may want to weigh in as well

elle-j · 2024-06-27T12:22:01Z

packages/realm/src/app-services/SyncSession.ts

+function toBindingProgressNotificationCallback(cb: ProgressNotificationCallback) {
+  if (isEstimateProgressNotification(cb)) {
+    return (transferredBytes: binding.Int64, transferrableBytes: binding.Int64, progressEstimate: number) =>
+      cb(progressEstimate);
+  } else {
+    // eslint-disable-next-line @typescript-eslint/no-unused-vars
+    return (transferredBytes: binding.Int64, transferrableBytes: binding.Int64, _: number) =>
+      cb(transferrableBytes, transferrableBytes);
+  }
+}


Nit: Argument name

The 2nd callback was accidentally called without transferredBytes.

Suggested change

function toBindingProgressNotificationCallback(cb: ProgressNotificationCallback) {

if (isEstimateProgressNotification(cb)) {

return (transferredBytes: binding.Int64, transferrableBytes: binding.Int64, progressEstimate: number) =>

cb(progressEstimate);

} else {

// eslint-disable-next-line @typescript-eslint/no-unused-vars

return (transferredBytes: binding.Int64, transferrableBytes: binding.Int64, _: number) =>

cb(transferrableBytes, transferrableBytes);

}

}

function toBindingProgressNotificationCallback(callback: ProgressNotificationCallback) {

if (isEstimateProgressNotification(callback)) {

return (_: binding.Int64, __: binding.Int64, progressEstimate: number) => callback(progressEstimate);

} else {

return (transferredBytes: binding.Int64, transferrableBytes: binding.Int64, _: number) =>

callback(transferredBytes, transferrableBytes);

}

}

elle-j · 2024-06-27T12:34:06Z

packages/realm/src/app-services/SyncSession.ts

-      (transferred, transferable) => callback(Number(transferred), Number(transferable)),
+      toBindingProgressNotificationCallback(callback),


elle-j · 2024-06-27T12:38:30Z

packages/realm/src/app-services/SyncSession.ts

-   * @param callback - Called with the following arguments:
-   * 1. `transferred`: The current number of bytes already transferred
-   * 2. `transferable`: The total number of transferable bytes (the number of bytes already transferred plus the number of bytes pending transfer)
+   * @param callback - see {@link DynamicProgressNotificationCallback} and {@link PartitionBasedSyncProgressNotificationCallback}


Perhaps something similar to this could suffice (mainly because the type itself exists by the parameter; so similarly to e.g. direction, we don't refer to the ProgressDirectionss available in the doc description here 👍 ). (Although it looks like we do for mode so I see that it's not really consistent 😛 )

Suggested change

* @param callback - see {@link DynamicProgressNotificationCallback} and {@link PartitionBasedSyncProgressNotificationCallback}

* @param callback - The function to call when the progress is updated.

elle-j · 2024-06-27T12:40:45Z

packages/realm/src/app-services/SyncSession.ts

+    if (this.config.flexible) {
+      assert(
+        isEstimateProgressNotification(callback),
+        `For flexible sync the callback can only take one argument - got ${callback.length} arguments`,
+      );
+    }


I think this may be a breaking change, since it has still been possible to call the API, and we haven't documented that it should have thrown before. Hmm, could we warn here instead?

I agree - this feels more breaking than a deprecation (which might be okay? but should be explicit and released accordingly) if it actually throws instead of simply warning.

We could update it to e.g.:

if (this.config.flexible && !isEstimateProgressNotificationCallback(callback)) { /* eslint-disable-next-line no-console */ console.warn("Deprecated progress notifier used. Please use 'EstimateProgressNotificationCallback' which accepts the estimate as the only argument."); }

But I think we could even skip the warning, since the warning is part of the deprecation in the docs. (We don't usually warn for other deprecations.)

For now I've updated this to just assert that it's a function.

elle-j · 2024-06-27T13:04:57Z

packages/realm/src/app-services/SyncSession.ts

@@ -53,13 +53,29 @@ export enum ProgressMode {
  ForCurrentlyOutstandingWork = "forCurrentlyOutstandingWork",
 }

-export type ProgressNotificationCallback =
+/** @deprecated */
+export type PartitionBasedSyncProgressNotificationCallback =


For the new public types that got introduced, they also need to be exported to users similar to this.

For deprecated ones such as the PBS-specific one, we list them below this line.

kraenhansen · 2024-06-28T09:17:42Z

integration-tests/tests/src/tests/sync/flexible.ts

+    before(async function (this: UserContext) {
+      // TODO: Investigate this.
+      // Use a new user for this test suite as not doing so causes some session issues in other tests
+      this.user = await this.app.logIn(Credentials.anonymous(false));
+    });


Are you aware of the authenticateUserBefore hook? If its important to avoid reusing, you could patch the function to take a boolean argument:

realm-js/integration-tests/tests/src/hooks/authenticate-user-before.ts

Line 24 in f8aefa2

this.user = this.app.currentUser || (await this.app.logIn(Credentials.anonymous()));

kraenhansen · 2024-06-28T09:18:43Z

integration-tests/tests/src/tests/sync/flexible.ts

+    beforeEach(async function (this: RealmContext & UserContext) {
+      // @ts-expect-error Using an internal API
+      this.realm = new Realm({
+        schema: [Person, Dog],
+        sync: {
+          flexible: true,
+          user: this.user,
+          _sessionStopPolicy: SessionStopPolicy.Immediately,
+        },
+      });
+    });


Is there a reason you're not using openRealmBeforeEach here?

Hm, I was moving back and forth between using that or my own as I had other cleanup hooks to check different things but now it seems like this can be fully covered byopenRealmBeforeEach, feel free to change it.

kraenhansen · 2024-06-28T09:20:44Z

integration-tests/tests/src/tests/sync/flexible.ts

+    it("only estimate callback is allowed", async function (this: RealmContext) {
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars, @typescript-eslint/no-empty-function
+      const callback = (estimate: number) => {};
+      this.realm.syncSession?.addProgressNotification(
+        Realm.ProgressDirection.Download,
+        Realm.ProgressMode.ForCurrentlyOutstandingWork,
+        callback,
+      );
+    });


What is this testing specifically? Passing a single argument arrow function as callback?

Yes, this has to do with deprecated API vs non-deprecated. Specifically, with sync 2 argument arrow function will error with flexible sync

gagik · 2024-06-28T09:34:10Z

Just tried to reply to some of the comments that may have been hard to guess without my input. Beyond that, going to be away so feel free to go ahead with what seems best 😄

elle-j

Great to see many tests here 🙂 Left some suggestions and thoughts

elle-j · 2024-07-03T07:24:06Z

integration-tests/tests/src/tests/sync/flexible.ts

+      );
+    });
+
+    it("old callback style is not allowed", async function (this: RealmContext) {


This should be updated (to be allowed) once the assertion from addProgressNotification() is removed in order to not break the API 👍

elle-j · 2024-07-03T07:32:58Z

integration-tests/tests/src/tests/sync/flexible.ts

+          await this.realm.syncSession?.uploadAllLocalChanges();
+          await this.realm.syncSession?.downloadAllServerChanges();


As this combo is used frequently, we could create a small helper.

elle-j · 2024-07-03T07:35:31Z

integration-tests/tests/src/tests/sync/flexible.ts

+          // TODO: This callback should not be called at this stage but seems flakey
+          // and gets called with 1.0 at times, likely because of a race condition.
+          expect(callback.notCalled || callback.calledWith(1.0)).is.true;


Wondering if we should still remove the callback.calledWith(1.0) though.

Suggested change

// TODO: This callback should not be called at this stage but seems flakey

// and gets called with 1.0 at times, likely because of a race condition.

expect(callback.notCalled || callback.calledWith(1.0)).is.true;

expect(callback.notCalled || callback.calledWith(1.0)).to.be.true;

elle-j · 2024-07-03T07:55:28Z

integration-tests/tests/src/tests/sync/flexible.ts

+
+          // TODO: Find a way to fix
+          // There should be at least one point where the progress is not yet finished.
+          // expect(callback.args.find(([estimate]) => estimate < 1)).to.not.be.undefined;


Suggested change

// expect(callback.args.find(([estimate]) => estimate < 1)).to.not.be.undefined;

// expect(callback.args.some(([estimate]) => estimate < 1)).to.be.true;

I'm unsure why your code there didn't work?

There seem to be another way with sinon as well:

expect(callback.calledWith(sinon.match(estimate => estimate < 1))).to.be.true;

A question regarding the comment though, is it true that we should always expect a number < 1? Or is it possible that for small uploads to sometimes only report the 1?

elle-j · 2024-07-03T08:00:49Z

integration-tests/tests/src/tests/sync/flexible.ts

+          // There should be at least one point where the progress is not yet finished.
+          // expect(callback.args.find(([estimate]) => estimate < 1)).to.not.be.undefined;
+
+          expect(callback.withArgs(1.0)).called;


Unsure if withArgs() is the correct usage here since that seems to create a new spy (according to their API docs). If using that, you probably want to put the called onto the spy instead (similar to the above where callback.notCalled is used):

expect(callback.withArgs(1.0).called).to.be.true

However, there are also these APIs:

callback.calledWith(..) callback.calledWithExactly(..)

elle-j · 2024-07-03T08:28:34Z

integration-tests/tests/src/tests/sync/sync-session.ts

 import { buildAppConfig } from "../../utils/build-app-config";
+import { spy } from "sinon";


Nit: Can move up to the 3rd party imports 🙂

elle-j · 2024-07-03T08:32:38Z

integration-tests/tests/src/tests/sync/sync-session.ts

-      expect(progressCalled).to.be.true;
+
+      const progressCallback = spy();
+      await Realm.open(config).progress(progressCallback);


elle-j · 2024-07-03T08:40:08Z

CHANGELOG.md

+* The callback for `realm.syncSession.addProgressNotification` will only take a single argument in the future: `estimate` (which supports both partition based and flexible sync). The old callback functionality is deprecated and will be removed. The estimate is roughly equivalent to an estimated value of `transferred / transferable` in the deprecated partition-based sync callback. ([#6256](/~https://github.com/realm/realm-js/issues/6256))  
+```ts
+/** New callback which supports both flexible and partition-based sync */
+realm.syncSession.addProgressNotification(
+  Realm.ProgressDirection.Upload,
+  ProgressMode.ReportIndefinitely,
+  (estimate) => console.log(`progress estimate: ${estimate}/1.0`),
+);
+/** @deprecated */
+realm.syncSession.addProgressNotification(
+  Realm.ProgressDirection.Upload,
+  ProgressMode.ReportIndefinitely,
+  (transferred, transferable) => console.log(`progress: ${(transferred / transferable)}/1.0`),
+);
+```


Nice info 🙂 I'm wondering if the info about the new API is better served along with the Enhancement entry and refer to that one instead? We could still mention deprecated API in the current section.

elle-j · 2024-07-03T08:40:19Z

CHANGELOG.md


 ### Enhancements
-* None
+* Added progress notifications support for flexible sync using a new callback argument. ([#6256](/~https://github.com/realm/realm-js/issues/6256))
+```ts


Suggested change

```ts

```typescript

elle-j · 2024-07-03T08:40:49Z

CHANGELOG.md

-* None
+* Added progress notifications support for flexible sync using a new callback argument. ([#6256](/~https://github.com/realm/realm-js/issues/6256))
+```ts
+realm.syncSession.addProgressNotification(


Suggested change

realm.syncSession.addProgressNotification(

realm.syncSession?.addProgressNotification(

elle-j · 2024-07-04T16:33:06Z

Note on Android tests: Still experiencing signature mismatch.

(Reference)

kraenhansen · 2024-07-05T05:50:01Z

@elle-j with #6769 merged, I'd expect the Android tests to run if this branch is rebased.

…()'.

Using link to the PR rather than the issue as we'll add more info there.

Needed on iOS.

cla-bot bot added the cla: yes label Jun 21, 2024

github-actions bot assigned gagik Jun 21, 2024

gagik changed the title ~~Add progress notifications and tests~~ RJS-2636: Add progress notifications and tests Jun 21, 2024

gagik commented Jun 21, 2024

View reviewed changes

gagik commented Jun 24, 2024

View reviewed changes

gagik marked this pull request as ready for review June 24, 2024 11:09

gagik marked this pull request as draft June 25, 2024 06:48

gagik force-pushed the gagik/progress-notifications branch from 945ca7a to 1908727 Compare June 26, 2024 07:39

gagik marked this pull request as ready for review June 26, 2024 09:10

gagik requested review from kraenhansen, elle-j and kneth June 26, 2024 09:12

gagik commented Jun 26, 2024

View reviewed changes

This was referenced Jun 26, 2024

RJS-2392: The new progress notification #6690

Closed

Progress notification for Flexible Sync #6257

Closed

gagik commented Jun 26, 2024

View reviewed changes

kneth approved these changes Jun 27, 2024

View reviewed changes

elle-j reviewed Jun 27, 2024

View reviewed changes

kraenhansen reviewed Jun 28, 2024

View reviewed changes

elle-j reviewed Jul 3, 2024

View reviewed changes

kneth and others added 2 commits July 5, 2024 08:37

wip

44ab5ad

WIP: useProgress

32d9dff

gagik and others added 25 commits July 5, 2024 08:43

Don't use relalmOpen

a296fab

Clear beforeEach

541c7fd

Manual unsubscribe

0440497

Fix package lock

d099d8f

Readd tests and manually unsubscribe

6e8542a

Comment out unreliable tests

2f54e78

Update progress listener token.

5e32152

Export and use 'isEstimateProgressNotificationCallback()'.

0eba7bc

Fix 'toBindingProgressNotificationCallback()'.

1e2f3ab

Update API docs and export new public types.

54bf4b8

Make 'emitProgress()' a method.

bf6120f

Fix binding to 'emitProgress()'.

31daca9

Allow PBS notifiers.

98cef97

Linting.

9d3e961

Update tests with 'openRealmBeforeEach()' and 'authenticateUserBefore…

31192ec

…()'.

Consistency updates.

3626323

Update test assertions.

2439c69

Add comments about flakyness.

a368710

Remove explicit 'realm.close()' in tests.

832da8f

Update name of token.

96cf82b

Add comments.

c09a6b2

Update CHANGELOG.

a443750

Using link to the PR rather than the issue as we'll add more info there.

Remove 'assert' library.

ac55fe4

Linting.

6ac8394

Add 'util' dependency in test for 'sinon'.

6f3811d

Needed on iOS.

elle-j force-pushed the gagik/progress-notifications branch from 3714a5d to 6f3811d Compare July 5, 2024 06:50

Update link to preferred callback in docs.

f27fff9

elle-j merged commit b4fd42b into main Jul 5, 2024
34 checks passed

elle-j deleted the gagik/progress-notifications branch July 5, 2024 08:31

github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2024

		@@ -167,18 +175,31 @@ export class ProgressRealmPromise implements Promise<Realm> {
		*/
		progress(callback: ProgressNotificationCallback): this {

		(transferred, transferable) => callback(Number(transferred), Number(transferable)),
		toBindingProgressNotificationCallback(callback),

	* @param callback - see {@link DynamicProgressNotificationCallback} and {@link PartitionBasedSyncProgressNotificationCallback}
	* @param callback - The function to call when the progress is updated.

		await this.realm.syncSession?.uploadAllLocalChanges();
		await this.realm.syncSession?.downloadAllServerChanges();

	// expect(callback.args.find(([estimate]) => estimate < 1)).to.not.be.undefined;
	// expect(callback.args.some(([estimate]) => estimate < 1)).to.be.true;

		import { buildAppConfig } from "../../utils/build-app-config";
		import { spy } from "sinon";

	realm.syncSession.addProgressNotification(
	realm.syncSession?.addProgressNotification(

RJS-2636: Add progress notifications and tests #6743

RJS-2636: Add progress notifications and tests #6743

Conversation

gagik commented Jun 21, 2024 • edited by elle-j Loading

What, How & Why?

☑️ ToDos

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gagik commented Jun 26, 2024

gagik Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gagik commented Jun 26, 2024 • edited Loading

kneth left a comment

Choose a reason for hiding this comment

elle-j left a comment

Choose a reason for hiding this comment

elle-j Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

elle-j Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elle-j Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

elle-j Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gagik Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

gagik commented Jun 28, 2024

elle-j left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elle-j commented Jul 4, 2024

kraenhansen commented Jul 5, 2024

gagik commented Jun 21, 2024 •

edited by elle-j

Loading

gagik Jun 26, 2024 •

edited

Loading

gagik commented Jun 26, 2024 •

edited

Loading

elle-j Jun 27, 2024 •

edited

Loading

elle-j Jun 27, 2024 •

edited

Loading

elle-j Jun 27, 2024 •

edited

Loading

elle-j Jun 27, 2024 •

edited

Loading

kraenhansen Jun 28, 2024 •

edited

Loading

gagik Jun 28, 2024 •

edited

Loading