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 initialSubscriptions sync config option to bootstrap initial set of flexible sync subscriptions #4561

Closed
wants to merge 15 commits into from

Conversation

tomduncalf
Copy link
Contributor

@tomduncalf tomduncalf commented May 6, 2022

What, How & Why?

This closes #4515, by introducing an initialSubscriptions option on the Realm's sync config, which allows you to bootstrap an initial set of subscriptions (using the same callback format as subscriptions.update). This will return a promise, resolved when the new subscriptions have been synchronised – i.e. if the user calls await Realm.open, it will not resolve until all the initial subscription data has been synchronised.

This is as part of the additive changes to flexible sync, in response to feedback that this a common use case. It's worth pointing out that this functionality should probably reside in core, but it was determined that there was not time to implement that, so for now each SDK is implementing it separately.

By default, the callback is only called when the Realm does not already exist, to allow the user to bootstrap an initial subscription set. The rerunOnStartup flag allows the user to rerun the callback every time the Realm is opened – the intended use case for this is that a user could implement e.g. a date range query in their subscription, which is updated every time the app is opened, in order to work around the lack of date range operators in RQL.

For example, to implement a subscription for "all people born in the last 30 days", which updates the date range every time you start the app, you could do something like:

const thirtyDaysAgo = new Date(new Date().setDate(new Date().getDate() - 30));
const config = {
  // ...
  sync: {
    flexible: true,
    user,
    initialSubscriptions: {
      update: (realm) => {
        realm.subscriptions.update((subs) => {
          subs.add(
            realm.objects("Person").filtered("dateOfBirth > $0", thirtyDaysAgo), 
            // This is a named subscription, so will replace any existing subscription with the same name
            { name: "People30Days" }
          );
        });
      },
      rerunOnStartup: true,
    },
  },
};

One gotcha to be aware of with this is that if you inline your config with the call to Realm.open and then try to use the realm variable that you are declaring inside the update callback, it won't work as it's not yet defined – you need to use the Realm instance provided as a callback argument. I'm not sure if there's anything we can do about this (except try to signpost it if we think it's a big issue?) and onFirstOpen presumably has the same issue.

const realm = await Realm.open({
  // ...
  sync: {
    initialSubscriptions: {
      update: () => {
        realm.... // <-- realm is undefined

Alternatives considered

I am open to feedback on the API for this, as I do feel it's perhaps a bit awkward that we are introducing a new concept that is actually quite similar to onFirstOpen. initialSubscriptions.update is presented as a callback for updating subscriptions, but in actual fact the user could do anything in this callback, so it could be a more generic thing. I felt like this was the "least bad" solution in the end – alternatives I considered, and the reasons for rejecting them are:

  • Reusing the existing onFirstOpen config option and making it awaitable (right now it does not return a promise), and adding a counterpart onOpen which is called every time the Realm opens to cover the rerunOnStartup case
    • onFirstOpen does not work for calling subscriptions.update, because the call to onFirstOpen from core is already in a write transaction. Therefore calling subscriptions.update from here fails, as update opens its own write transaction.
    • As onFirstOpen is implemented using initialization_function in core, this would require changes to core to add the equivalent for onOpen (which requires touch a lot of files to pass it through the various configs and layers) and to make it awaitable. We would also need to change the semantics of onFirstOpen and/or update with regard to write transactions.
    • This seemed like too complex a change, especially as the intention is for this initial subscription functionality to eventually be moved to core.
  • Have the implementation in the SDK like it is in this PR, but make it a more generic solution (e.g. add a top level callback on the config called onOpen, with a rerunOnOpenOnStartup flag, which lets the user do initial work they want of any kind – essentially an SDK-side awaitable replacement for onFirstOpen)
    • This would create an awkward duplication with onFirstOpen – would it be a replacement for it? Or are there some semantic differences?
    • Is this even something we want to add? It would need some proper design and consideration before adding a new generic concept.
    • For the initial subscriptions use case (which is currently the only one, and the one we want to streamline), this would require users to remember to return realm.subscriptions.waitForSynchronization(); from their onOpen callback, otherwise we wouldn't wait for synchronisation.
      • We could add a waitForSynchronisation flag to do this implicitly, but that feels messy.
  • Implementing the solution purely in JS in lib/extensions.js
    • My initial implementation did this, which was simpler in terms of the code, but did not work for calls to new Realm – right now all our config options are supported for both new Realm and Realm.open so I thought we should uphold this.

One other idea I did have was:

  • Make the initialSubscriptions.update callback internally wrapped in subscriptions.update and change its signature to update: (subs: MutableSubscriptionSet, realm: Realm)
    • This would perhaps make it a bit more ergonomic for users as they don't need to call realm.subscriptions.update, and it would guide them more clearly to just using this callback for updating subscriptions
    • I felt like this might be introducing unnecessary complexity and was somewhat redundant as you already have the realm, but looking at it with fresh eyes maybe this is actually a nicer solution? Thoughts welcome!

☑️ ToDos

  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 🚦 Tests
  • 📱 Check the React Native/other sample apps work if necessary
  • 📝 Public documentation PR created or is not necessary
  • 💥 Breaking label has been applied or is not necessary

If this PR adds or changes public API's:

  • typescript definitions file is updated
  • jsdoc files updated
  • Chrome debug API is updated if API is available on React Native

@cla-bot cla-bot bot added the cla: yes label May 6, 2022
@tomduncalf tomduncalf force-pushed the td/flx-bootstrap-initial-subs branch from 9a4832c to e5c10d9 Compare May 6, 2022 09:57
@tomduncalf tomduncalf changed the title Td/flx bootstrap initial subs Add initialSubscriptions sync config option to bootstrap initial set of flexible sync subscriptions May 9, 2022
@tomduncalf tomduncalf force-pushed the td/flx-bootstrap-initial-subs branch from e8f17e3 to d994a1e Compare May 9, 2022 10:58
@tomduncalf tomduncalf requested review from kneth, takameyer and kraenhansen and removed request for kneth May 9, 2022 10:58
@tomduncalf tomduncalf marked this pull request as ready for review May 9, 2022 10:58
@tomduncalf tomduncalf requested a review from kneth May 9, 2022 10:58
@tomduncalf
Copy link
Contributor Author

Spotted something that needs changing, temporarily closing

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.

Flx Sync: Bootstrap initial set of subscriptions on open and make awaitable
1 participant