Skip to content

fix: make setContextField and removeContextField be async#254

Merged
gastonfournier merged 1 commit intoUnleash:mainfrom
nitpum:fix-set-context-field-async
Aug 4, 2025
Merged

fix: make setContextField and removeContextField be async#254
gastonfournier merged 1 commit intoUnleash:mainfrom
nitpum:fix-set-context-field-async

Conversation

@nitpum
Copy link
Contributor

@nitpum nitpum commented Jul 2, 2025

About the changes

setContextField and removeContextField uses updateToggles inside like updateContext but both setContextField and removeContextField don't await updateToggles. Lead to a case that will get toggle that use old context value.

This fix is add async to both function like updateContext does. Now we can await both function

}

this.updateToggles();
await this.updateToggles();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should just return this.updateToggles() here?
@kwasniew what do you think about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

In flutter we have Promise equivalent: https://github.com/Unleash/unleash-flutter-sdk/blob/main/lib/unleash_proxy_client_flutter.dart#L350

I think we should incorporate this change as it is now

@gastonfournier gastonfournier moved this from New to Investigating in Issues and PRs Jul 2, 2025
@stale
Copy link

stale bot commented Aug 1, 2025

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 1, 2025
@nitpum
Copy link
Contributor Author

nitpum commented Aug 2, 2025

@gastonfournier any update on this?

@stale stale bot removed the stale label Aug 2, 2025
@kwasniew kwasniew self-requested a review August 4, 2025 07:57
Copy link
Contributor

@kwasniew kwasniew left a comment

Choose a reason for hiding this comment

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

I think it's a good change and is pretty much aligned with out flutter SDK that I was working on in the past: https://github.com/Unleash/unleash-flutter-sdk/blob/main/lib/unleash_proxy_client_flutter.dart#L350

@github-project-automation github-project-automation bot moved this from Investigating to Approved PRs in Issues and PRs Aug 4, 2025
@gastonfournier
Copy link
Contributor

I think it's a good change and is pretty much aligned with out flutter SDK that I was working on in the past: https://github.com/Unleash/unleash-flutter-sdk/blob/main/lib/unleash_proxy_client_flutter.dart#L350

Thanks! I'm still unsure if this is the way to go... having update toggles as a side effect of setting context fields. Making it event-driven is kind of the same: you can't await updateContext, you'd have to await for an event to arrive. There's an alternative approach in the Java SDK where we allow you to manually call updateToggles which you can await.

But given @kwasniew is fine with this, I'm also fine with this solution as is.

@gastonfournier
Copy link
Contributor

@gastonfournier any update on this?

Sorry! Summer happens :D Thanks for your patience, I'm merging this now

@gastonfournier gastonfournier merged commit 2693832 into Unleash:main Aug 4, 2025
4 checks passed
@github-project-automation github-project-automation bot moved this from Approved PRs to Done in Issues and PRs Aug 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants