Hi @lsj8706 . Thank you for your contribution. Our main Swift guy is Out of office until Tuesday March 4th, so you'll have to wait until then at the earliest for a proper reply here.

@lsj8706

I see what you are trying to do here. I'm wondering if there is a way to do this without requiring a major update. As we are changing the default behavior of the StorageProvider, everyone who currently has their own implementation would have to change it.

Also, is batch a better term than reset? I was initially confused because of the terminology.

er term than reset? I was in

Hello! Thank you for your thoughtful comments.
In my opinion, "batch" and "reset" represent different operations. The current implementation completely overwrites the existing storage, which aligns more with a "reset" operation. For it to be appropriately named "batch," I believe the implementation should merge the received parameters into the storage rather than replacing the entire storage.
That said, I'm certainly open to changing the name from "reset" to something more appropriate if there's a better term that accurately describes this operation.

@FredrikOseberg
I understand your concerns.

However, this change is intended to resolve existing performance issues, which I believe cannot be solved with the current interface alone. Current users are incurring the cost of n write operations when storing n Toggles in Storage, and this is a critical issue that must be addressed eventually.

Although this will result in a major update, it seems better to resolve it during this opportunity when the scope of impact can be minimized as much as possible.

@FredrikOseberg I understand your concerns.

However, this change is intended to resolve existing performance issues, which I believe cannot be solved with the current interface alone. Current users are incurring the cost of n write operations when storing n Toggles in Storage, and this is a critical issue that must be addressed eventually.

Although this will result in a major update, it seems better to resolve it during this opportunity when the scope of impact can be minimized as much as possible.

With the provided DictionaryStorageProvider the current mode of operations should be fine, as the insert is entirely in memory and inserting into a dict is a O(1) operation. I can definitely see that this would be a problem for custom storage providers that are writing to disk, and I'd like to solve that problem. Do you have any benchmarks that can illustrate the performance gain on your provider with these changes?

Generally, making a major release is more work for us, because we need to provide upgrade guides and documentation around the changes that led to the major update. It also puts some mental overhead on our users because they need to understand the changes and potentially make changes. Not saying I won't make a major release for this, but we try to be careful the number of majors that we do. Out of curiosity, would it be possible to do something like:

public protocol StorageProvider {
    func set(value: Toggle?, key: String)
    @objc optional func reset(keyedValues: [String: Toggle])
    func value(key: String) -> Toggle?
    func clear()
}

if storageProvider.responds(to: #selector(StorageProvider.reset(keyedValues:))) {
            storageProvider.reset?(keyedValues: keyedValues)
        } else {
            // If reset is not supported, set each toggle individually
            for (key, value) in keyedValues {
                storageProvider.set(value: value, key: key)
            }
        }

@FredrikOseberg
Thank you for your thoughtful feedback regarding the major update. We can address this without requiring a major update by implementing it in the following way:

public protocol StorageProvider {
    func set(value: Toggle?, key: String)
    func reset(keyedValues: [String: Toggle])
    func value(key: String) -> Toggle?
    func clear()
}

extension StorageProvider {
  func reset(keyedValues: [String: Toggle]) {
    for (key, value) in keyedValues {
      set(value: value, key: key)
    }
  }
}

By using Swift's protocol default implementation for the reset method, existing users won't need to implement the reset method separately. This addresses your concern since only those who need it can implement a custom reset method, while others can rely on the default implementation.

@FredrikOseberg Thank you for your thoughtful feedback regarding the major update. We can address this without requiring a major update by implementing it in the following way:

public protocol StorageProvider {
    func set(value: Toggle?, key: String)
    func reset(keyedValues: [String: Toggle])
    func value(key: String) -> Toggle?
    func clear()
}

extension StorageProvider {
  func reset(keyedValues: [String: Toggle]) {
    for (key, value) in keyedValues {
      set(value: value, key: key)
    }
  }
}

By using Swift's protocol default implementation for the reset method, existing users won't need to implement the reset method separately. This addresses your concern since only those who need it can implement a custom reset method, while others can rely on the default implementation.

@lsj8706

Aiden Lee, thanks a lot for discussing this with me. I apologize for the time this has taken, but we have many SDKs and we try to make sure they are aligned as much as possible in how they work to reduce the cognitive load on users and ourselves. I've had a look at how our other SDKs handle this scenario in the StorageProviders and they all take the entire featureset and set it in the storageprovider in one go. I'm not sure why we opted for iterating over the features here, but it's clear that it's the wrong approach.

I'm leaning towards doing a major update and changing the interface of the StorageProvider, but in that case I think we should just replace the set method entirely and have the set method do what you propose instead of adding a new method. It would keep the StorageProvider small and the interface more in line with other SDKs, and we'll remove this anti-pattern as a possibility. Here's an example of how it works in other SDKs:

• Node: https://github.com/Unleash/unleash-client-node/blob/main/src/repository/storage-provider-in-mem.ts#L6-L9
• Javascript: https://github.com/Unleash/unleash-proxy-client-js/blob/main/src/storage-provider-inmemory.ts

I'd also love a test for this functionality if you have the time.

What do you think? Could you make those changes to this PR?

@lsj8706

Aiden Lee, thanks a lot for discussing this with me. I apologize for the time this has taken, but we have many SDKs and we try to make sure they are aligned as much as possible in how they work to reduce the cognitive load on users and ourselves. I've had a look at how our other SDKs handle this scenario in the StorageProviders and they all take the entire featureset and set it in the storageprovider in one go. I'm not sure why we opted for iterating over the features here, but it's clear that it's the wrong approach.

I'm leaning towards doing a major update and changing the interface of the StorageProvider, but in that case I think we should just replace the set method entirely and have the set method do what you propose instead of adding a new method. It would keep the StorageProvider small and the interface more in line with other SDKs, and we'll remove this anti-pattern as a possibility. Here's an example of how it works in other SDKs:

• Node: https://github.com/Unleash/unleash-client-node/blob/main/src/repository/storage-provider-in-mem.ts#L6-L9
• Javascript: https://github.com/Unleash/unleash-proxy-client-js/blob/main/src/storage-provider-inmemory.ts

I'd also love a test for this functionality if you have the time.

What do you think? Could you make those changes to this PR?

@FredrikOseberg
Thank you for your kind explanation. I agree that we need to unify the behavior with other SDKs.

I've checked the code from other SDKs that you attached, and I also think it's appropriate to change the implementation of the set method.

Therefore, I've modified the implementation to store all Toggles at once in the set method. I've tested the modified implementation and confirmed that it passes without errors. 2d91e8d

Additionally, I've added information about the injectability of the Poller to the README, as this information was missing.
a3ae1f3

Please check and review. Thank you!