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: support multiple environment caching #252

Merged
merged 18 commits into from
Oct 9, 2024

Conversation

kyle-ssg
Copy link
Member

@kyle-ssg kyle-ssg commented Sep 17, 2024

See #246

@kyle-ssg
Copy link
Member Author

@oluizcarvalho This is published under [email protected]!

@oluizcarvalho
Copy link
Contributor

@oluizcarvalho This is published under [email protected]!

Excellent, I'm going to test it in my projects.

…-caching

# Conflicts:
#	lib/flagsmith-es/package.json
#	lib/flagsmith/package.json
#	lib/react-native-flagsmith/package.json
#	types.d.ts
# Conflicts:
#	lib/flagsmith-es/package.json
#	lib/flagsmith/package.json
#	lib/react-native-flagsmith/package.json
#	test/cache.test.ts
@kyle-ssg
Copy link
Member Author

@oluizcarvalho This is published under [email protected]!

Excellent, I'm going to test it in my projects.

@oluizcarvalho Could I confirm everything's going well with this ? I'd like to publish a release with this soon 🚀

@oluizcarvalho
Copy link
Contributor

oluizcarvalho commented Sep 24, 2024

@oluizcarvalho This is published under [email protected]!

Excellent, I'm going to test it in my projects.

@oluizcarvalho Could I confirm everything's going well with this ? I'd like to publish a release with this soon 🚀

Yes, I finished the tests.

I noticed that in this documentation we mention the old key that will be replaced, it would be interesting to change.

All tests were successful with two different keys being used in the same browser.

I haven't tested the scenario of having two identical keys with different instances in the same browser, but I don't think it would be a problem either.

@kyle-ssg
Copy link
Member Author

@oluizcarvalho This is published under [email protected]!

Excellent, I'm going to test it in my projects.

@oluizcarvalho Could I confirm everything's going well with this ? I'd like to publish a release with this soon 🚀

Yes, I finished the tests.

I noticed that in this documentation we mention the old key that will be replaced, it would be interesting to change.

Yep we can do that our end!

All tests were successful with two different keys being used in the same browser.

Amazing work!

I haven't tested the scenario of having two identical keys with different instances in the same browser, but I don't think it would be a problem either.

Actually I can maybe think of 1 case, if one instance was identifying and the other wasn't, the api responses would combat cache.

@kyle-ssg
Copy link
Member Author

kyle-ssg commented Oct 1, 2024

I've been thinking about this more and I think the cache key should have the identity in it, it makes sense we do this as part of the same release

@oluizcarvalho
Copy link
Contributor

oluizcarvalho commented Oct 2, 2024

I've been thinking about this more and I think the cache key should have the identity in it, it makes sense we do this as part of the same release

Since it is a major version, I think it makes sense to include this other solution together. I still can't understand how it would work, but I imagine that you wanted to make the cache key as
prefix_default + envId + userId(optional)

I believe that it wouldn't be that complex to adapt, but if a lot of restructuring is needed, you could separate it into a new issue, since today customizing the cache key (cacheOptions.storageKey) seems to be a way to solve the scenario

@oluizcarvalho
Copy link
Contributor

@kyle-ssg any feedback on this, I would like to have access to the official version?

@kyle-ssg
Copy link
Member Author

kyle-ssg commented Oct 9, 2024

Hey @oluizcarvalho I'll take a look at how difficult this would be today, though I agree RE the custom key. The aim will be to do a release today 👍

…-caching

# Conflicts:
#	flagsmith-core.ts
#	test/cache.test.ts
…t-caching' into feat/support-multiple-environment-caching

# Conflicts:
#	test/cache.test.ts
@kyle-ssg
Copy link
Member Author

kyle-ssg commented Oct 9, 2024

Considering this more, I think we should keep behaviour as it is for now since it could be desirable to retrieve identity cache when an identity isn't provided to flagsmith.init. I've adjusted the key retrieval to be a function so we could easily add this in future.

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.

Approved with a minor formatting gripe.

@@ -590,7 +585,7 @@ const Flagsmith = class {
this.api = state.api || this.api || defaultAPI;
this.flags = state.flags || this.flags;
this.evaluationContext = state.evaluationContext || this.evaluationContext,
this.evaluationEvent = state.evaluationEvent || this.evaluationEvent;
this.evaluationEvent = state.evaluationEvent || this.evaluationEvent;
Copy link
Contributor

Choose a reason for hiding this comment

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

odd formatting?

@kyle-ssg kyle-ssg merged commit 8951a32 into main Oct 9, 2024
1 check passed
@kyle-ssg kyle-ssg deleted the feat/support-multiple-environment-caching branch October 9, 2024 12:33
@matthewelwell matthewelwell mentioned this pull request Dec 18, 2024
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