Support `NSUbiquitousKeyValueStore`

hank121314 commented 1 year ago

Summary

This PR fixes: #75

Create a Defaults.iCloud class which facilitates the synchronization of keys between the local UserDefaults and NSUbiquitousKeyValueStore. This class utilizes a TaskQueue to execute synchronization tasks sequentially, ensuring proper order and coordination. Internally, Defaults.iCloud contains an AtomicSet that indicates the synchronization status of each key. This set prevents observation triggers when synchronization is being executed.

And I also create a new protocol Defaults.KeyValueStore which contains essential properties for synchronizing a key value store. It can be useful when mocking a test or even support another key value data source.

Thanks

Please feel free to critique, and thanks for your code review 😄 . Any feedback on the PR is welcomed and appreciated 🙇 !

sindresorhus commented 1 year ago

Wow. This is really awesome! 🎉

hank121314 commented 1 year ago

It is challenging to change the Key-Value Observing (KVO) mechanism of UserDefaults to asyncstream.

Here is a demonstration of how Defaults.iCloud works.

flowchart TD
    A("UserDefaults \n Changes \n ex.Defaults[key] = 0")
    B("NSUbiquitousKeyValueStore \n didChangeExternallyNotification")
    C("Defaults.iCloud.syncKeys()")
    D[\"Push \n Synchronization \n Task \n Sequentially"/]
    E(["Executing Tasks \n Task.detached"])
    F(["All Tasks Complete"])
    A & B & C --> D --> E -- "await Defaults.iCloud.sync()" --> F

If we change the observation to asyncstream, there is a need to create a task to consume the asyncstream. Since the user's change is not in the same task as the asyncstream, it is difficult to ensure that the synchronization is completed synchronously.

ex.

Task.detached {
    for await change in updates {
        print("\(change)")
    }
}

Defaults[key] = 0
Defaults[key] = 1
await Defaults.iCloud.sync()

// => 0
// await Defaults.iCloud.sync() get call
// => 1

sindresorhus commented 1 year ago

Understood. It's fine. We can use KVO then. Just add a todo to look into using async stream when Swift supports custom executors.

hank121314 commented 1 year ago

I believe it might be worth considering waiting for the release of Swift 5.9, which will include the implementation of Custom Actor Executors. Once it's available, we can transition all Key-Value Observing (KVO) and notifications to utilize AsyncStream.

sindresorhus commented 1 year ago

Is the custom actor executors feature backdeployed? If not, we would have to target macOS 14, which does not make sense.

hank121314 commented 10 months ago

Is the custom actor executors feature backdeployed? If not, we would have to target macOS 14, which does not make sense.

SerialExecutor is back deployed to macOS 10.15+, but some of its methods is deprecated and new methods only target to macOS 14+. And I am also misreading the proposal, we must wait until the Specifying Task executors is completed.

hank121314 commented 10 months ago

Btw, my old MacBook can not install macOS 14+ and Xcode 15+, so I can not develop some features which related to swift 5.9 😭 . Will buy the M3 MacBook until it release in Taiwan.

Kinark commented 9 months ago

Any updates on this? It's a really awesome PR :/

hank121314 commented 9 months ago

I want to check a behavior when initialized with iCloud: true. Currently, we push synchronization from the local data source triggered by suite.register. However, I am concerned that this may overwrite remote data when installing on a new device. Perhaps it would be better to add UserDefaults observation after suite.register and let the user manually call Defaults.syncWithoutWaiting (which compares timestamps between the two data sources). What do you think?

sindresorhus commented 9 months ago

However, I am concerned that this may overwrite remote data when installing on a new device. Perhaps it would be better to add UserDefaults observation after suite.register and let the user manually call Defaults.syncWithoutWaiting (which compares timestamps between the two data sources). What do you think?

I would prefer if users don't have to call anything manually. Can we wait until the sync (fetching remote data) is done before pushing the local data to remote?

Maybe also look at how Zephyr handles this.

hank121314 commented 9 months ago

Seems Zephyr also facing the same issue. https://github.com/ArtSabintsev/Zephyr/issues/50#issuecomment-774680007

I have a thought to resolve this issue, but it might involve some trade-offs. The current implementation only records the latest timestamp when data is set in the data source, using only one key. If we can record the latest timestamp for all keys, we can ensure whether a specific key needs to be synced or not. However, the trade-off is a 2x space allocation with both remote and local storage.

sindresorhus commented 9 months ago

Local storage is not a problem, but synced storage is a problem as it's limited to 1 Kb in total. Is there a way to use timestamps for each key but not sync them?

hank121314 commented 9 months ago

Is there a way to use timestamps for each key but not sync them?

Oh, this is a good idea. For this use case, where a user has a fresh new device with some data in iCloud Storage, we can consider the iCloud Storage as the latest data since there is no corresponding timestamp key in their local storage.

Will try to implement this in a few days! Thank you 🙌 !

hank121314 commented 8 months ago

I tried the method mentioned above but noticed that it may not cover all use cases.

If a user has multiple Defaults.Keys with iCloud set to true, after syncing the first key, the remote timestamp is recorded. When syncing the second key, we cannot be sure about the state of the data source.

For example.

// Choose `local` data source, sync `name` to `remoteStorage`, record `remoteStorage` timestamp.
let name = Defaults.Key<String>("name", default: "0", iCloud: true)
// `remoteStorage` have timestamp record and local timestamp is empty so we will choose `remote` data source, sync `quality` to `localStorage`, but the data source should be `local` instead.
let quality = Defaults.Key<Double>("quality", default: 0.0, iCloud: true)

Considering the scenario, it seems necessary to maintain timestamp records in both remoteStorage and localStorage to accurately determine the appropriate data source 😭 .

And considering the limitation of 1024 keys in remoteStorage, a potential solution could be to consolidate the value and timestamp into a single key?

For example.

class iCloudKey {
    let value: Any?
    let timestamp: Date?
}

sindresorhus commented 8 months ago

Considering the scenario, it seems necessary to maintain timestamp records in both remoteStorage and localStorage to accurately determine the appropriate data source 😭 .

You are right. I don't see any way around this either.

hank121314 commented 8 months ago

So it is necessary to maintain the timestamp in both storages.

For localStorage, where there is no limitation, we can introduce another key with a prefix (__DEFAULTS__synchronizeTimestamp) to record the timestamp. As for remoteStorage, considering the 1024 keys limitation, we can combine the value and timestamp into a single key and stored it in remoteStorage.

Is this an acceptable solution?

sindresorhus commented 8 months ago

Correct

For the remote storage we should try to make it take the least amount of space, because there is also a total limit of 1MB storage. Maybe we can convert the timestamp to fixed-length data, serialize the value to data too, combine them, and store them both in one Data value? To deserialize, we would just slice of the first part, which would be a fixed length data for the timestamp and then deserialize the rest as data. I'm open to other ideas.

hank121314 commented 8 months ago

Maybe we can convert the timestamp to fixed-length data, serialize the value to data too, combine them, and store them both in one Data value?

I believe there might be some effort required in serializing the value into data. Considering that the value sent to remote storage can be guaranteed as a UserDefaults-acceptable object (retrieved from local storage, which is UserDefaults), we can simplify the process by storing a two-length array. The first element would be the timestamp, and the second element would be the value (ex. [Date(), 1]).

sindresorhus commented 8 months ago

The first element would be the timestamp, and the second element would be the value (ex. [Date(), 1]).

👍

hank121314 commented 7 months ago

PR updated! Do following changes:

The timestamp will now be based on the key to ensure accurate source detection.
To ensure consistency between two storage's timestamp, each synchronization task will be assigned a unique timestamp with @TaskLocal.
Will automatically sync the key after Defaults.iCloud.add

Please feel free to review it again! Many thanks!

Kinark commented 6 months ago

@sindresorhus please, review their last changes, this would help Winston so much lmfao

sindresorhus commented 6 months ago

@Kinark Please don't comment if you have nothing to add to the discussion. If you want to see this finished sooner, help with reviewing this is welcome :)