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

Add Flexible Sync subscribe/unsubscribe APIs #5772

Merged
merged 29 commits into from
Jun 1, 2023
Merged

Add Flexible Sync subscribe/unsubscribe APIs #5772

merged 29 commits into from
Jun 1, 2023

Conversation

elle-j
Copy link
Contributor

@elle-j elle-j commented Apr 27, 2023

What, How & Why?

Additions:

  • Experimental APIs: Adds the possibility to subscribe and unsubscribe directly to and from a Results instance via Results.subscribe() and the Results.unsubscribe().
    • A SubscriptionOptions can be passed to Results.subscribe() which has been extended to include:
      • A WaitForSync behavior specifying how to wait or not wait for subscribed objects to be downloaded before the returned promise resolves.
      • A maximum waiting timeout before resolving.
  • Adds the instance method MutableSubscriptionSet.removeUnnamed() allowing for the removal of unnamed subscriptions only.

Example subscribe():

const peopleOver20 = await realm
  .objects("Person")
  .filtered("age > 20")
  .subscribe({
    name: "peopleOver20",
    behavior: WaitForSync.FirstTime, // Default
    timeout: 2000,
  });
// ...
peopleOver20.unsubscribe();

Example removeUnnamed():

await realm.subscriptions.update((mutableSubs) => {
  mutableSubs.removeUnnamed();
});

This closes #5760.

☑️ ToDos

  • 📝 Changelog entry
  • 🚦 Tests
  • 🔀 Executed flexible sync tests locally if modifying flexible sync
  • 📝 Public documentation PR created or is not necessary

@elle-j elle-j changed the title [Don't merge yet] Add a Flexible Sync Subscribe API Add Flexible Sync Subscribe APIs May 1, 2023
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
packages/realm/src/app-services/MutableSubscriptionSet.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@takameyer takameyer left a comment

Choose a reason for hiding this comment

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

LGTM! Just have a few pondering questions/considerations

CHANGELOG.md Outdated Show resolved Hide resolved
integration-tests/tests/src/tests/sync/flexible.ts Outdated Show resolved Hide resolved
@elle-j elle-j marked this pull request as draft May 9, 2023 09:50
@elle-j elle-j marked this pull request as ready for review May 25, 2023 14:46
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.

It still looks good to me. I have only minor comments.

packages/realm/src/Results.ts Outdated Show resolved Hide resolved
packages/realm/src/Results.ts Show resolved Hide resolved
expect(this.realm.subscriptions).to.have.length(1);
});

it("waits for objects to sync the first time only for different 'Results' instances", async function (this: RealmContext) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the test title is a little confusing for me, but this means that WaitForSync applies to the query and not to the results, am I correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is due to how we determine if we should wait for sync or not. If, for instance, the subscription is not named, we check if it already exists by query like you said.

I'll modify the test description and can also add some comments in the test 👍

Copy link
Contributor

@papafe papafe left a comment

Choose a reason for hiding this comment

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

Looks good! And nice test coverage

packages/realm/src/Results.ts Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
});

expect(subs).to.have.length(1);
expect([...subs][0].queryString).to.equal("age > 10");
Copy link
Member

Choose a reason for hiding this comment

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

This would be equivalent and perhaps a bit simpler?

Suggested change
expect([...subs][0].queryString).to.equal("age > 10");
const [ sub ] = subs;
expect(sub.queryString).to.equal("age > 10");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That can be a small but good refactor we could add in a separate PR (perhaps for our test refactor ticket), because the previous pattern is also used in other places so would be good to change all of them together.

@elle-j elle-j changed the title Add Flexible Sync Subscribe APIs Add Flexible Sync subscribe/unsubscribe APIs Jun 1, 2023
@elle-j elle-j merged commit 3a00265 into main Jun 1, 2023
@elle-j elle-j deleted the lj/flx-subscribe branch June 1, 2023 09:44
papafe added a commit that referenced this pull request Jun 7, 2023
* main:
  Fix User.callFunction JSDoc to match the v11+ API (#5768)
  Add Flexible Sync subscribe/unsubscribe APIs (#5772)
  Fix warning for deprecated namespace setting method in Android (#5862)
  Update install-test-react-native.yml (#5848)
  Update package-unit-tests.yml to add ccache and ninja (#5837)
  Enable cleartext traffic in android test app to make tests work in release builds

# Conflicts:
#	CHANGELOG.md
papafe added a commit that referenced this pull request Jun 7, 2023
* fp/update-core-13.13:
  Corrected changelog
  Updated changelog
  Fix User.callFunction JSDoc to match the v11+ API (#5768)
  Add Flexible Sync subscribe/unsubscribe APIs (#5772)
  Fix warning for deprecated namespace setting method in Android (#5862)
  Update install-test-react-native.yml (#5848)
  Update package-unit-tests.yml to add ccache and ninja (#5837)
  Enable cleartext traffic in android test app to make tests work in release builds
  Removed unused
  Updated changelog

# Conflicts:
#	packages/realm/bindgen/vendor/realm-core
#	packages/realm/src/index.ts
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flex Sync Subscribe API
6 participants