-
Notifications
You must be signed in to change notification settings - Fork 107
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
Dynamic config resolver schemas and types #793
Dynamic config resolver schemas and types #793
Conversation
COMPUTED: ConfigSyncResolverDefinition<[string], [string]>; | ||
DYNAMIC: ConfigAsyncResolverDefinition<undefined, number>; | ||
GRPC_SERVICES_NAMES: ConfigEnvDefinition<true>; | ||
DYNAMIC: ConfigAsyncResolverDefinition<undefined, number, 'serverStart'>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these are test configs, it would be good to add a comment above them like you did on Line 41.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the best option would be to have a separate config file specifically for tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to remove all of the unused/test configs in my second upcomming PR
@@ -30,17 +36,32 @@ const dynamicConfigs: { | |||
GRPC_SERVICES_NAMES: { | |||
env: 'NEXT_PUBLIC_CADENCE_GRPC_SERVICES_NAMES', | |||
default: 'cadence-frontend', | |||
isPublic: true, | |||
}, | |||
// For testing purposes | |||
DYNAMIC: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these are test configs anyway, "ASYNC" and "SYNC" would be more meaningful than "DYNAMIC" and "COMPUTED"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing it in my second upcoming PR
}; | ||
|
||
export type ConfigEnvDefinition = { | ||
export type ConfigEnvDefinition<IsPublic extends boolean = false> = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit (as discussed): a union type would be nicer here, just so that we can write ConfigEnvDefinition<'public'>
instead of ConfigEnvDefinition<true>
Co-authored-by: Adhitya Mamallan <adi1998is@gmail.com>
Summary
Adding validation schemas for config resolvers (args/return types) & use those schemas to validate the input/output of resolvers on runtime.
Add
evaluateOn
flag to configs to define when resolvers are being executed. It supportsrequest
&serverStart
.Add
isPublic
flag to configs to identify which configs are accessible from client code.Changes
getTransformedConfigs
totransformConfigs
for better testing (doing the same forget-config-value.ts
in upcomming PR)evaluateOn
&isPublic
flagsUpcoming plans
get-config-value.ts