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(core): add feature flag caching #21

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

Tim-53
Copy link
Collaborator

@Tim-53 Tim-53 commented Jun 2, 2023

Feature flags are cached by the core and refetched if expired.

@vercel
Copy link

vercel bot commented Jun 2, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
abby-opensource ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 13, 2023 7:29pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
abby-docs ⬜️ Ignored (Inspect) Visit Preview Jul 13, 2023 7:29pm

@Tim-53 Tim-53 changed the title Feat/core/add feature flag caching Feat:core add feature flag caching Jun 2, 2023
@Tim-53 Tim-53 changed the title Feat:core add feature flag caching Feat(core): add feature flag caching Jun 2, 2023
packages/core/src/index.ts Outdated Show resolved Hide resolved
packages/core/src/index.ts Outdated Show resolved Hide resolved
@@ -62,7 +68,7 @@ export class Abby<
Tests extends Record<string, ABConfig>
> {
private log = (...args: any[]) =>
this.config.debug ? console.log(`core.Abby`, ...args) : () => {};
this.config.debug ? console.log(`core.Abby`, ...args) : () => { };
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we use console?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We dont have a proper logger integrated right now , so is this used to log info in debugging mode atm.

packages/core/src/index.ts Outdated Show resolved Hide resolved
* helper function to make testing easier
*/
refetchFlags() {
this.loadProjectData();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
this.loadProjectData();
return this.loadProjectData();

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

loadProjectData() has return type void.

if (!flagTime) return this.#data.flags[key];
const now = new Date();
if (flagTime.getTime() <= now.getTime()) {
this.refetchFlags()
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is async.. so without waiting the return down below may be old though 🤷

packages/core/src/index.ts Outdated Show resolved Hide resolved
packages/core/src/index.ts Outdated Show resolved Hide resolved
packages/core/src/index.ts Outdated Show resolved Hide resolved
packages/core/src/index.ts Outdated Show resolved Hide resolved
private testDevtoolOverrides: Map<keyof Tests, Tests[keyof Tests]["variants"][number]> =
new Map();

private flagDevtoolOverrides: Map<FlagName, boolean> = new Map();
Copy link
Collaborator

Choose a reason for hiding this comment

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

guess this shouldn't be "just booleans" anymore, right?
the fix should be another PR :)

* @param key name of the featureflag
* @returns value of flag
*/
getValidFlag<F extends FlagName>(key: F, refetch: boolean | undefined) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this may break every call in every lib. if this is not intended, maybe do refetch?: boolean instead. :)

@cstrnt
Copy link
Member

cstrnt commented Jul 20, 2023

I honestly feel like this feature is kinda against our business model :D We potentially want as many requests against our API so we can charge for that. I personally think that if users want to avoid this, they can cache it themselves in memory and live with stale data

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