-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add formal support for extract/hydrate of prefab config #73
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -134,7 +134,24 @@ export class Prefab { | |
| return this.load(); | ||
| } | ||
|
|
||
| get configs(): { [key: string]: Config } { | ||
| extract(): Record<string, Config["value"]> { | ||
| return Object.entries(this._configs).reduce( | ||
| (agg, [key, value]) => ({ | ||
| ...agg, | ||
| [key]: value.value, | ||
| }), | ||
| {} as Record<string, Config["value"]> | ||
| ); | ||
| } | ||
|
|
||
| hydrate(rawValues: RawConfigWithoutTypes | EvaluationPayload): void { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure there is an actual use-case for supporting
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe The trick for something like this is understandable if you think of experiment evaluation. You might want to only send the exposure event when the actual flag is evaluated, but SSR you're going to eval everything in order to give values to the front end. The customer isn't really "evaluating" all those flags, so we don't want to fire telemetry as SSR hydration time, we really should fire the telemetry when/if the actually prefab.get() is called. But same thing for trying to think about zombie flag detection. you don't want to say "all these flags are in use" just because you're hydrating.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that all makes sense... That said, in the context of this change, we aren't considering them "evaluated" as far as I can tell. What I implement hydrates the cache using the same this.setConfig (now private) method. It's also worth calling out that SSR may have evaluated some of the flags in the cache. In that case, I'm assuming you guys already handle flushing the telemetry, and there is no need to pass it to the FE to ensure it's flushed. Otherwise, I agree there is no need to support the evaluations API here, in which case I will update the hydrate method to explicitly not support that, both in typing + in implementation. Thoughts?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I investigated a bit further based on what comes back from the API when initializing the client as well as how evaluations are tracked and shipped. The stuff in the explicit cache actually doesn't include any of the actual evaluation data. That is tracked in a different data object entirely. So there is no harm in actually supporting the evaluation payload as it exists in the cache. This raises more questions for me than anything in terms of why this is actually needed in the cache at all. We can have that conversation another day. |
||
| this.setConfigPrivate(rawValues); | ||
| } | ||
|
|
||
| get configs(): Record<string, Config> { | ||
| // Log message in yellow without adding chalk dependency | ||
| console.warn("\x1b[33m%s\x1b[0m", 'Deprecated: Use "prefab.extract" instead'); | ||
|
|
||
| return this._configs; | ||
| } | ||
|
|
||
|
|
@@ -174,7 +191,7 @@ export class Prefab { | |
| const bootstrapContext = new Context(prefabBootstrap.context); | ||
|
|
||
| if (this.context.equals(bootstrapContext)) { | ||
| this.setConfig({ evaluations: prefabBootstrap.evaluations }); | ||
| this.setConfigPrivate({ evaluations: prefabBootstrap.evaluations }); | ||
| return Promise.resolve(); | ||
| } | ||
| } | ||
|
|
@@ -185,7 +202,7 @@ export class Prefab { | |
| return this.loader | ||
| .load() | ||
| .then((rawValues: any) => { | ||
| this.setConfig(rawValues as EvaluationPayload); | ||
| this.setConfigPrivate(rawValues as EvaluationPayload); | ||
| }) | ||
| .finally(() => { | ||
| if (this.pollStatus.status === "running") { | ||
|
|
@@ -254,6 +271,13 @@ export class Prefab { | |
| } | ||
|
|
||
| setConfig(rawValues: RawConfigWithoutTypes | EvaluationPayload) { | ||
| // Log message in yellow without adding chalk dependency | ||
| console.warn("\x1b[33m%s\x1b[0m", 'Deprecated: Use "prefab.hydrate" instead'); | ||
|
|
||
| this.setConfigPrivate(rawValues); | ||
| } | ||
|
|
||
| private setConfigPrivate(rawValues: RawConfigWithoutTypes | EvaluationPayload) { | ||
| this._configs = Config.digest(rawValues); | ||
| this.loaded = true; | ||
| } | ||
|
|
@@ -274,7 +298,7 @@ export class Prefab { | |
| return undefined; | ||
| } | ||
|
|
||
| const config = this.configs[key]; | ||
| const config = this._configs[key]; | ||
|
|
||
| const value = config?.value; | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.