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

fix: Don't use cached traits to initialize sdk state #282

Merged
merged 5 commits into from
Jan 17, 2025

Conversation

tiagoapolo
Copy link
Contributor

@tiagoapolo tiagoapolo commented Jan 15, 2025

#273

As discussed with @kyle-ssg we're not going to cache traits anymore, but for the sake of simplicity we're still saving in local storage because we're storing the everything from the evaluation context but not using it to initialize sdk inner states

  • This pull request fix cached traits overwriting server values by not updating traits inner sdk state from cached traits.
  • Traits used in init will overwrite previous server value, preserving existing behavior as documented here

@tiagoapolo tiagoapolo requested a review from kyle-ssg January 16, 2025 12:16
@tiagoapolo
Copy link
Contributor Author

@kyle-ssg I wonder if we still need to keep these tests:

@kyle-ssg
Copy link
Member

@kyle-ssg I wonder if we still need to keep these tests:

@kyle-ssg I wonder if we still need to keep these tests:

Yeah with this approach we don't really care about cached traits at all so we don't need the tests

Copy link
Contributor

@matthewelwell matthewelwell left a comment

Choose a reason for hiding this comment

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

This looks good to me. Are there any considerations we need to make before releasing this, or are we happy that this is just a fix we can release in a patch?

@tiagoapolo
Copy link
Contributor Author

tiagoapolo commented Jan 17, 2025

This looks good to me. Are there any considerations we need to make before releasing this, or are we happy that this is just a fix we can release in a patch?

From my last chat with Kyle we agreed that no one is using that behavior as part of their business logic, also it would be pretty weird, and perhaps a really tiny bit fraction of users are considering caching traits. In summary, no considerations needed.

@tiagoapolo tiagoapolo merged commit 94bcd3e into main Jan 17, 2025
1 check passed
@tiagoapolo tiagoapolo deleted the fix/remove--traits-from-cache--273 branch January 17, 2025 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants