Skip to content

feat(sdk): improve configurable_prop types #16698

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat(sdk): improve configurable_prop types #16698

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1fd58bd
feat(sdk): improve configurable_prop types
TheBestMoshe May 16, 2025
8063b1b
removed unused code
TheBestMoshe May 16, 2025
a6dd5f4
refactor: removed non-existing types
TheBestMoshe May 16, 2025
961ffed
chore: updated changelog
TheBestMoshe May 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 111 additions & 30 deletions packages/sdk/src/shared/component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,11 @@ type BaseConfigurableProp = {
withLabel?: boolean;
};

// XXX fix duplicating mapping to value type here and with PropValue
type Defaultable<T> = { default?: T; options?: T[]; };
type Defaultable<T> = { default?: T; options?: T[] };

Comment on lines +57 to +58
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Defaultable is too restrictive for “array-of-X” property kinds

Defaultable<T> hard-codes options?: T[].
When you reuse it for array property kinds ("number[]", "integer[]", "boolean[]", etc.) you end up with:

default?: number;        // should be number[]
options?: number[];      // should be number[]

Consequently default and PropValue<"number[]"> become incompatible and the compiler will not prevent the error because the mismatch sits in different structural types (prop definition vs. configured value).

Consider introducing a dedicated helper for array props or inlining default / options:

-export type ConfigurablePropNumberArray = BaseConfigurableProp & {
-  type: "number[]";
-} & Defaultable<number>;
+export type ConfigurablePropNumberArray = BaseConfigurableProp & {
+  type: "number[]";
+  default?: number[];
+  options?: number[];
+};

Same adjustment is required for ConfigurablePropIntegerArray, ConfigurablePropBooleanArray, and ConfigurablePropDiscordChannelArray.
Failing to do so will allow mis-typed defaults to slip into published schema and break user tooling at runtime.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
type Defaultable<T> = { default?: T; options?: T[] };
export type ConfigurablePropNumberArray = BaseConfigurableProp & {
type: "number[]";
default?: number[];
options?: number[];
};
🤖 Prompt for AI Agents 
In packages/sdk/src/shared/component.ts around lines 57 to 58, the
Defaultable<T> type is too restrictive because it defines options as T[] and
default as T, which causes type mismatches for array property kinds like
"number[]". To fix this, create a separate helper type specifically for array
properties where both default and options are arrays of T (e.g., default?: T[];
options?: T[][] or appropriate array types), and update
ConfigurablePropIntegerArray, ConfigurablePropBooleanArray, and
ConfigurablePropDiscordChannelArray accordingly to use this new helper, ensuring
type compatibility and preventing mis-typed defaults.

// Define a type for string-based property options
export type StringPropOptionObject = { label?: string; value: string };
export type StringPropOption = string | StringPropOptionObject;

export type ConfigurablePropAlert = BaseConfigurableProp & {
type: "alert";
Expand All @@ -69,7 +72,9 @@ export type ConfigurablePropApp = BaseConfigurableProp & {
type: "app";
app: string;
};
export type ConfigurablePropBoolean = BaseConfigurableProp & { type: "boolean"; };
export type ConfigurablePropBoolean = BaseConfigurableProp & {
type: "boolean";
} & Defaultable<boolean>;
export type ConfigurablePropInteger = BaseConfigurableProp & {
type: "integer";
min?: number;
Expand All @@ -81,54 +86,128 @@ export type ConfigurablePropObject = BaseConfigurableProp & {
export type ConfigurablePropString = BaseConfigurableProp & {
type: "string";
secret?: boolean;
} & Defaultable<string>;
default?: string;
options?: StringPropOption[];
};
export type ConfigurablePropStringArray = BaseConfigurableProp & {
type: "string[]";
secret?: boolean; // TODO is this supported
} & Defaultable<string[]>; // TODO
// | { type: "$.interface.http" } // source only
// | { type: "$.interface.timer" } // source only
// | { type: "$.service.db" }
// | { type: "data_store" }
// | { type: "http_request" }
// | { type: "sql" } -- not in component api docs!
default?: string[];
options?: StringPropOption[];
};
export type ConfigurablePropNumber = BaseConfigurableProp & {
type: "number";
min?: number;
max?: number;
} & Defaultable<number>;
export type ConfigurablePropNumberArray = BaseConfigurableProp & {
type: "number[]";
} & Defaultable<number>;
export type ConfigurablePropIntegerArray = BaseConfigurableProp & {
type: "integer[]";
} & Defaultable<number>;
export type ConfigurablePropBooleanArray = BaseConfigurableProp & {
type: "boolean[]";
} & Defaultable<boolean>;
export type ConfigurablePropDiscordChannel = BaseConfigurableProp & {
type: "$.discord.channel";
[key: string]: unknown;
} & Defaultable<string>;
export type ConfigurablePropDiscordChannelArray = BaseConfigurableProp & {
type: "$.discord.channel[]";
[key: string]: unknown;
} & Defaultable<string>;
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Array prop defaults / options currently accept scalar values

The four definitions below inherit Defaultable<scalar> and therefore expose the wrong surface in editors and generated JSON schemas:

  • ConfigurablePropNumberArray
  • ConfigurablePropIntegerArray
  • ConfigurablePropBooleanArray
  • ConfigurablePropDiscordChannelArray

Either:

  1. Create type DefaultableArray<T> = { default?: T[]; options?: T[] } and use that, or
  2. Inline the correct fields as shown in the previous comment.

This minor refactor removes an entire class of silent runtime bugs while keeping the public API intact.

🤖 Prompt for AI Agents 
In packages/sdk/src/shared/component.ts between lines 98 and 119, the array
types ConfigurablePropNumberArray, ConfigurablePropIntegerArray,
ConfigurablePropBooleanArray, and ConfigurablePropDiscordChannelArray
incorrectly use Defaultable with scalar types, causing defaults and options to
accept scalar values instead of arrays. To fix this, define a new type
DefaultableArray<T> with default and options as optional arrays of T, then
replace Defaultable<T> with DefaultableArray<T> in these array type definitions
to ensure defaults and options are arrays, preventing runtime bugs.


export type ConfigurablePropInterfaceHttp = BaseConfigurableProp & {
type: "$.interface.http";
[key: string]: unknown;
};
export type ConfigurablePropInterfaceTimer = BaseConfigurableProp & {
type: "$.interface.timer";
[key: string]: unknown;
};
export type ConfigurablePropServiceDb = BaseConfigurableProp & {
type: "$.service.db";
[key: string]: unknown;
};
export type ConfigurablePropDataStore = BaseConfigurableProp & {
type: "datastore";
[key: string]: unknown;
};
export type ConfigurablePropHttpRequest = BaseConfigurableProp & {
type: "http_request";
[key: string]: unknown;
};
export type ConfigurablePropSql = BaseConfigurableProp & { type: "sql" };

export type ConfigurableProp =
| ConfigurablePropAlert
| ConfigurablePropAny
| ConfigurablePropApp
| ConfigurablePropBoolean
| ConfigurablePropBooleanArray
| ConfigurablePropInteger
| ConfigurablePropIntegerArray
| ConfigurablePropNumber
| ConfigurablePropNumberArray
| ConfigurablePropObject
| ConfigurablePropString
| ConfigurablePropStringArray
| (BaseConfigurableProp & { type: "$.discord.channel"; });
| ConfigurablePropDiscordChannel
| ConfigurablePropDiscordChannelArray
| ConfigurablePropInterfaceHttp
| ConfigurablePropInterfaceTimer
| ConfigurablePropServiceDb
| ConfigurablePropDataStore
| ConfigurablePropHttpRequest
| ConfigurablePropSql;

export type ConfigurableProps = Readonly<ConfigurableProp[]>;

export type PropValue<T extends ConfigurableProp["type"]> = T extends "alert"
? never
: T extends "any"
? any // eslint-disable-line @typescript-eslint/no-explicit-any
: T extends "app"
? { authProvisionId: string; }
: T extends "boolean"
? boolean
: T extends "integer"
? number
: T extends "object"
? object
: T extends "string"
? string
: T extends "string[]"
? string[] // XXX support arrays differently?
: never;
? any // eslint-disable-line @typescript-eslint/no-explicit-any
: T extends "app"
? { authProvisionId: string }
: T extends "boolean"
? boolean
: T extends "boolean[]"
? boolean[]
: T extends "integer"
? number
: T extends "integer[]"
? number[]
: T extends "object"
? object
: T extends "string"
? string
: T extends "string[]"
? string[]
: T extends "number"
? number
: T extends "number[]"
? number[]
: T extends "$.discord.channel"
? string
: T extends "$.discord.channel[]"
? string[]
: T extends
| "$.interface.http"
| "$.interface.timer"
| "$.service.db"
| "datastore"
| "http_request"
| "sql"
? unknown
: never;

export type ConfiguredProps<T extends ConfigurableProps> = {
[K in T[number] as K["name"]]?: PropValue<K["type"]>
[K in T[number] as K["name"]]?: PropValue<K["type"]>;
};

// as returned by API (configurable_props_json from `afterSave`)
export type V1Component<T extends ConfigurableProps = any> = { // eslint-disable-line @typescript-eslint/no-explicit-any
export type V1Component<T extends ConfigurableProps = ConfigurableProps> = {
name: string;
key: string;
version: string;
Expand All @@ -137,7 +216,9 @@ export type V1Component<T extends ConfigurableProps = any> = { // eslint-disable
component_type?: string;
};

export type V1DeployedComponent<T extends ConfigurableProps = any> = { // eslint-disable-line @typescript-eslint/no-explicit-any
export type V1DeployedComponent<
T extends ConfigurableProps = ConfigurableProps,
> = {
id: string;
owner_id: string;
component_id: string;
Expand Down Expand Up @@ -171,4 +252,4 @@ export type V1EmittedEvent = {
* The event's unique ID.
*/
id: string;
}
};
Loading