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: make variable key types overridable via module declaration #1002

Merged
merged 3 commits into from
Dec 2, 2024

Conversation

ajwootto
Copy link
Contributor

@ajwootto ajwootto commented Nov 28, 2024

Based on #1001

Update the type definitions of our SDKs to support overriding the definition of allowable variables via declare module interface merging. This makes it so that something like this becomes fully type-safe:

declare module '@devcycle/types' {
    interface CustomVariableDefinitions {
        flag: 'a' | 'b'
    }
}
const test = useVariableValue('flag', 'b')
// typeof test is `'a' | 'b'`

// type error here, 'c' is not a valid value for 'flag'
const test = useVariableValue('flag', 'c')

// type error here, 'unknown' is not a valid key
const test2 = useVariableValue('unknown', false)

Copy link

vercel bot commented Nov 28, 2024

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

Name Status Preview Comments Updated (UTC)
js-sdks-web-elements ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 2, 2024 5:47pm
js-sdks-with-provider ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 2, 2024 5:47pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
js-sdks-next-js-page-router ⬜️ Ignored (Inspect) Dec 2, 2024 5:47pm

@ajwootto ajwootto marked this pull request as ready for review November 29, 2024 22:19
@ehis-042

This comment was marked as spam.

// eslint-disable-next-line @typescript-eslint/no-empty-interface
export interface CustomVariableDefinitions {}
type DynamicBaseVariableDefinitions =
keyof CustomVariableDefinitions extends never
Copy link
Member

Choose a reason for hiding this comment

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

How does the never work here? How would this statement ever be evaluated?

Copy link
Member

Choose a reason for hiding this comment

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

interesting:

1. keyof CustomVariableDefinitions extends never: This expression checks if the CustomVariableDefinitions interface has any keys.
- keyof CustomVariableDefinitions produces a union of the keys of the CustomVariableDefinitions interface. If the interface has no keys, this results in the never type.
- extends never checks if this union is never, which would mean that CustomVariableDefinitions has no keys.

Copy link
Contributor

Choose a reason for hiding this comment

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

For some more explanation even though I think you probably got the gist of it from the ChatGPT/Copilot explanation (this was my code from the previous PR);

The interface CustomVariableDefinitions can be augmented via a process called "Declaration Merging" which has an entire page dedicated to it in the TS docs (meaning it's not all that straightforward 😆 ).

So because of this CustomVariableDefinitions could be either an empty object if the client code hasn't augmented it, or an object with keys if it has been augmented. Meaning the interface kind of acts like a generic - the non declaration merging way that people would usually do in their own client code would be type DynamicBaseVariableDefinitions<TDefs> = keyof TDefs extends never ? ...

Using CustomVariableDefinitions on it's own would require users to always do the declaration merging or any "variable key" argument would be defined as key: never.
Particularly for those non-typescript users who just use types "invisibly" as IDE autocomplete or just the "automatic get out of my way" typescript users we want the key parameters to be string typed.

With all that, you could read the type definition in natural language as;

if CustomVariableDefinitions has not been augmented by Declaration merging (ie keys are never)
then VariableDefinitions should be `Record<string, string | number | boolean>`
otherwise use the strict typed version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants