Skip to content

Rewrite type declarations#42

Open
JELoohuis wants to merge 1 commit intoathombv:masterfrom
JELoohuis:add-generic-types
Open

Rewrite type declarations#42
JELoohuis wants to merge 1 commit intoathombv:masterfrom
JELoohuis:add-generic-types

Conversation

@JELoohuis
Copy link
Copy Markdown

The data types did not yet reflect the actual type of data they contained, so I made the class definition generic and added a new DataTypes definition using it.

I also took the liberty to rewrite the other definitions based on the types we have built at @athombv/drenso, based on the JS source and runtime introspection.

@RobinBol RobinBol self-requested a review April 13, 2026 11:57
@RobinBol RobinBol self-assigned this Apr 13, 2026
Comment thread index.d.ts
Comment on lines +18 to +24
data8 : DataType<number>,
data16: DataType<number>,
data24: DataType<number>,
data32: DataType<number>,
data40: DataType<number>,
data48: DataType<number>,
data56: DataType<number>,
Copy link
Copy Markdown
Contributor

@RobinBol RobinBol Apr 16, 2026

Choose a reason for hiding this comment

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

  1. data40/48/56 typed incorrectly as DataType<number> - should be DataType<Buffer>
  2. data64 missing from type declarations

Comment thread index.d.ts
single: DataType<number>,
double: DataType<number>,

octstr: DataType<string>,
Copy link
Copy Markdown
Contributor

@RobinBol RobinBol Apr 16, 2026

Choose a reason for hiding this comment

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

octstr typed as DataType<string> - should be DataType<Buffer>

Comment thread index.d.ts
Comment on lines +94 to +95
Array0: <Type>(type: Type) => DataType<Array<Type>>,
Array8: <Type>(type: Type) => DataType<Array<Type>>,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you call DataTypes.Array8(DataTypes.uint8), Type infers as DataType<number>, producing DataType<Array<DataType<number>>>. The actual runtime value is number[]. Should be:

Array0: <T>(type: DataType<T>) => DataType<Array<T>>

Comment thread index.d.ts
data48: DataType<number>,
data56: DataType<number>,

bool: DataType<boolean>,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
bool: DataType<boolean>,
bool: DataType<boolean | null>,

Comment thread index.d.ts
toJSON: () => T;
toBuffer: (buffer?: Buffer, index?: number) => Buffer;

class BitMap<Flags extends string | null> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
class BitMap<Flags extends string | null> {
class Bitmap<Flags extends string | null> {

Comment thread index.d.ts
Comment on lines +28 to +35
map8 : <Flags extends string | null>(flags: Array<Flags>) => DataType<BitMap<Flags>>,
map16: <Flags extends string | null>(flags: Array<Flags>) => DataType<BitMap<Flags>>,
map24: <Flags extends string | null>(flags: Array<Flags>) => DataType<BitMap<Flags>>,
map32: <Flags extends string | null>(flags: Array<Flags>) => DataType<BitMap<Flags>>,
map40: <Flags extends string | null>(flags: Array<Flags>) => DataType<BitMap<Flags>>,
map48: <Flags extends string | null>(flags: Array<Flags>) => DataType<BitMap<Flags>>,
map56: <Flags extends string | null>(flags: Array<Flags>) => DataType<BitMap<Flags>>,
map64: <Flags extends string | null>(flags: Array<Flags>) => DataType<BitMap<Flags>>,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The PR types map8 as (flags: Array) - a single array argument. But node-zigbee calls it with spread args:

DataTypes.map8('flag1', 'flag2', 'flag3')  // actual usage
DataTypes.map8(['flag1', 'flag2', 'flag3'])  // what PR expects
Runtime accepts (...arg) so spread args is correct. The type should be:

map8: <Flags extends string | null>(...flags: Array<Flags>) => DataType<BitMap<Flags>>

Comment thread index.d.ts
toBuffer: (buffer: Buffer, index?: number) => Buffer;
}

function Struct<Defs extends Record<string, DataType<any>>> (name: string, defs: Defs, opts?: {encodeMissingFieldsBehavior?: 'default' | 'skip'}): StaticStruct<Defs>;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The new generic parameter Defs extends Record<string, DataType<any>> is a breaking change for existing consumers. In node-zigbee, all Structs use the old pattern:

Struct<ZdoActiveEndpointsResponse>('name', { status: ZdoStatusCodesDataType, ... })

This passes the output type as the generic, not the defs record. With this change, TS errors on every single Struct call (17+ errors across zigbee and zstack packages).

Comment thread index.d.ts
declare module "@athombv/data-types" {
interface DataTypeInterface {
declare module "@athombv/data-types" {
class DataType<Value> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The DataType constructor is not exposed in the type declarations, but consumers create custom DataType instances directly. For example in node-zigbee's zdoStructs.mts:

new DataType(NaN, 'ZdoComplexDescriptor', 0, toBufFn, fromBufFn)

This currently errors with Expected 0 arguments, but got 5. The constructor needs to be declared:

constructor(id: number, shortName: string, length: number, toBuf: (...) => number, fromBuf: (...) => Value, ...args: unknown[]);

Comment thread index.d.ts

type StructInstance<Defs extends Record<string, import('@athombv/data-types').DataType<any>>> = StructProperties<Defs> & {
toJSON: () => StructProperties<Defs>;
toBuffer: (buffer: Buffer, index?: number) => Buffer;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

buffer should be optional here. The runtime allocates a buffer internally when called without arguments (toBuffer() with no args), and existing consumers rely on this:

const buf = instance.toBuffer(); // valid at runtime, errors with these types

Should be:

toBuffer: (buffer?: Buffer, index?: number) => Buffer;

Comment thread index.d.ts
Comment on lines +55 to +57
enum8 : <Flags extends string>(flags: Record<Flags, number>) => DataType<Flags>,
enum16: <Flags extends string>(flags: Record<Flags, number>) => DataType<Flags>,
enum32: <Flags extends string>(flags: Record<Flags, number>) => DataType<Flags>,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same issue as the map types - the runtime uses (...arg) rest params, and some consumers spread additional args. In node-zigbee/zstack this causes TS2556: A spread argument must either have a tuple type or be passed to a rest parameter and TS2554: Expected 1 arguments, but got N errors.

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.

2 participants