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

Feat/async start and updateContext overloads #90

Conversation

benjamin-es-hall
Copy link
Contributor

About the changes

This adds async overloads for the start and updateContext methods so that they can safely be called from async context in modern swift concurrency. MainActor requirement is needed for the timer within the Poller to fire on the RunLoop.

Fairly sure this should close #88
Closes #88

Important files

Discussion points

Copy link
Contributor

@FredrikOseberg FredrikOseberg left a comment

Choose a reason for hiding this comment

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

@benjamin-es-hall Thank you! I think it looks sensible, but how will this affect the minimum version of Unleash? In the past we changed away from the usage of ObservableObject on the base in order to support iOS 12. See #34

@benjamin-es-hall
Copy link
Contributor Author

benjamin-es-hall commented Oct 8, 2024

@benjamin-es-hall Thank you! I think it looks sensible, but how will this affect the minimum version of Unleash? In the past we changed away from the usage of ObservableObject on the base in order to support iOS 12. See #34

No problem!

Oh I didn't notice UnleashClient was exposed only to iOS13 and that was the differencing factor to UnleashClientBase.

In that case, the async methods I've put in Base I've marked for only available to iOS13 and shouldn't have any issues, but I think we could equally just move them into UnleashClient. Thoughts?

Would you also be able to explain why ObservableObject is used? It's use case is not clear to me without an example since any changes seem to be propagated via the swiftEventBus and notification center anyway?

@benjamin-es-hall
Copy link
Contributor Author

@FredrikOseberg just pushed up a commit to move the methods to the UnleashClient class where it already specifies iOS13 requirement

@FredrikOseberg
Copy link
Contributor

@benjamin-es-hall Thank you! I think it looks sensible, but how will this affect the minimum version of Unleash? In the past we changed away from the usage of ObservableObject on the base in order to support iOS 12. See #34

No problem!

Oh I didn't notice UnleashClient was exposed only to iOS13 and that was the differencing factor to UnleashClientBase.

In that case, the async methods I've put in Base I've marked for only available to iOS13 and shouldn't have any issues, but I think we could equally just move them into UnleashClient. Thoughts?

Would you also be able to explain why ObservableObject is used? It's use case is not clear to me without an example since any changes seem to be propagated via the swiftEventBus and notification center anyway?

Observable object was initially used to allow UI changes to propagate automatically when a feature flag updated. Without it, from my understanding, you'll have to trigger the UI update manually yourself. This was done to keep consistency with how our other client side SDKs handle flag changes.

@FredrikOseberg FredrikOseberg enabled auto-merge (squash) October 10, 2024 10:40
Copy link
Contributor

@FredrikOseberg FredrikOseberg left a comment

Choose a reason for hiding this comment

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

LGTM

@FredrikOseberg FredrikOseberg merged commit dae3e2c into Unleash:main Oct 10, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Polling does not work when start called from async context
2 participants