SDK Review

[toddbaert]

I've opened this issue as a place to centralize review of the Swift SDK built by Spotify. Thanks to all that have worked on this. The community is excited to have this contribution!

I've gone through the code as best I can (as a Swift novice) and here are my observations so far:

Spec compliance

These are the things that seem to not yet be implemented from a specification perspective. We may not need to implement all of them for a 1.0, but they're worth pointing out!

FlagMetadata

• FlagMetadata is missing on the FlagEvaluationDetails and ProviderEvaluation. It is pretty easy to implement, and can be done later. See related point below.

Multiple provider support

• This is not implemented, see here, here. There's some additional background here.

Provider initialization/shutdown

• This is partially implemented.
  • The provider interface needs a "shutdown" function to clean up timers, threads, connections, etc. See here, and here
  • The API needs a shutdown function to shut down all providers gracefully when the application terminates, see here.
  • This gets a bit more complex with multiple providers

Named provider (aka multi-provider) support

• This is not implemented, see here, here. There's some additional background here.

Events

• This is not implemented, see here.

General recommendations and observations

These are things that aren't really about spec compliance, but general points I've noticed:

The Features protocol is synchronous

• This is what I would expect, just as it is in our client JS implementation: https://github.com/open-feature/js-sdk/blob/main/packages/client/src/evaluation/evaluation.ts. I'm only pointing it out because it's an important choice that should be noted and considered. I think it makes sense for client situations ("static" contexts), wherein we expect values to be fetched during initialization and updated during context changes, and then cached. I just want to make sure that providers will likely be able to fulfill this contract. Is it likely that providers won't need to do any async work (besides when they initialize and react to context changes?). If so, I think this implementation is good.

Value

• Similarly to Kotlin, I'm wondering if there's a "simpler" alternative for representing arbitrary structured data in a type-safe, serializable way here. For example, in TS/JS this is just a plain object with the type restrictions. In Go it's a map[string][interface{}]. I would recommend whatever the "standard" way of representing a JSON object is in Swift, if one exists.

Metadata

• I recommend calling this ProviderMetadata or something else. We regretted this naming in other SDKs, since we found number use-cases for a generic Metadata object that carries arbitrary information (FlagMetadata, and EventMetadata, for instance).

setProvider is an async function

• This is similar to the discussion in the Kotlin SDK (suspend). In JS we made the intentional choice to make this method non-async. The initialize() method is being called in here, but it's not awaited. When the init completes, READY/ERROR is emitted based on if the method was successful or not (see here).
• I'm OK with making this an async function in theory, but I think it's a bit inconsistent with the events API, and our other SDKs. Another thing to consider is that client instances (which may be in a completely different place in the codebase) can't await (or react to errors thrown by) the async setProvider; events are needed for this, which may make it less sensible to mark this async.

Proposal

Like the Kotlin SDK, this looks very good to me. I see most of these issues as non-blocking, and good for initial release. I would just consider how to prevent breaking changes as much as possible. Specifically, consider the points above about Value, the async setProvider, and the synchronous Features protocol.

It would also be very helpful to use the template readme here, for a 1:1 comparison between other SDKs.

I would love to hear input from the contributors from Spotify and the OpenFeature community in general on my proposal above.

[Calibretto]

Thank you again for such a thorough review!

As with the Kotlin SDK, we're assuming that the spec compliance is a low priority (for now) and we'll focus on other areas. The provider events, however, are probably the highest priority out of these.

Synchronous Features Protocol - we've been discussing this all week and we agree that the current synchronous implementation is the way to go (there may be issues if we do ever decide to include a dynamic context implementation, but at the moment we have no plans for that).

Value - we can certainly take a look at simplifying this, there's likely some language features we can leverage to make this easier to use / understand.

Metadata - we'll get this renamed.

Async setProvider - much of the same discussion on the Kotlin side with suspend applies here too and we'd like to continue using async/await here. Our main reasoning is that without the provider events this is the best way to handle it and we can revisit it, if necessary once we have these events.

We'd like to get some of the smaller fixes done ASAP and we'll take a look at the bigger things over the next few days. We'll keep you updated with what changes we're hoping to get done before the first release of the SDK.

Thanks.

[Calibretto]

Hi @toddbaert - we've just pushed some updates to the SDK and released v0.2.5 - we've changed and added some things based on your feedback.

Main Changes:

• Renamed the Metadata protocols to better distinguish them.
• Implemented events for the provider and for client/provider pairs.
• Updated the README.md to follow the template and included details about the provider events.

Adding the provider events has allowed us to remove any asynchronous functions from the public API, meaning setting the provider or changing the context are now synchronous.

We feel this is in a good position now and hope you feel the same, please review these updates and let us know if there's anything else you'd like changed or added before OF adopts it.

Thank you.

[toddbaert]

Hi @toddbaert - we've just pushed some updates to the SDK and released v0.2.5 - we've changed and added some things based on your feedback.

Main Changes:

• Renamed the Metadata protocols to better distinguish them.
• Implemented events for the provider and for client/provider pairs.
• Updated the README.md to follow the template and included details about the provider events.

Adding the provider events has allowed us to remove any asynchronous functions from the public API, meaning setting the provider or changing the context are now synchronous.

We feel this is in a good position now and hope you feel the same, please review these updates and let us know if there's anything else you'd like changed or added before OF adopts it.

Thank you.

I've reviewed the above, and I'm definitely in favor of the change! I hope users find this useful. Thanks again for the time and efforts.

[toddbaert]

The management PR is merged, and the repo permissions look good.

Maintainers can feel free to merge the SDK into that repo whenever. Just be careful not to unintentionally release anything you don't mean to ! 😅 If you have issues or need anything from org admins (secrets, config changes, etc, please ping me, or @beeme1mr or anyone else on the TC/GC)

I'll be creating issues in the upcoming days/weeks for some housekeeping items, and maintainers should feel free to do the same.

Thanks again to all who helped with this! We can also close this issue whenever if it's no longer useful.