retejs / rete

JavaScript framework for visual programming
https://retejs.org
MIT License
10.17k stars 653 forks source link

Enforce String Key Types for Connection Class Properties. #668

Closed Necmttn closed 11 months ago

Necmttn commented 1 year ago

Issue: The current type definitions for the sourceOutput and targetInput properties in the Connection class are inferred as keyof Source['outputs'] and keyof Target['inputs'] respectively. In TypeScript, the keyof operator resolves to a union of string | number | symbol as object keys in JavaScript can be of any of these types. This leads to a type incompatibility issue when these properties are expected to be of type string in certain usage scenarios, as seen in the attached error screenshot

CleanShot 2566-10-22 at 17 04 34@2x

Proposed Solution: To enforce the string type for these properties and avoid the type incompatibility issue, a utility type StringKeyof<T> is introduced to filter out only the string keys from a type. The Connection class is then updated to use this utility type for the sourceOutput and targetInput properties, ensuring they are typed as string.

type StringKeyof<T> = Extract<keyof T, string>;

export declare class Connection<Source extends Node, Target extends Node> implements ConnectionBase {
    sourceOutput: StringKeyof<Source['outputs']>;
    targetInput: StringKeyof<Target['inputs']>;
    // ... rest of the class ...
}

This adjustment aligns the type definitions with the expectation that all keys in Source['outputs'] and Target['inputs'] are strings, and resolves the type error encountered in the described scenario.

Attached are the error logs and a minimal reproduction case demonstrating the issue and the effectiveness of the proposed solution in resolving the type error.

Ni55aN commented 1 year ago

can you share the createControlFlowEngine implementation?

Necmttn commented 1 year ago

Sure it's just a wrapper;

export const createControlFlowEngine = <
  Schemes extends ControlFlowEngineScheme
>() => {
  const engine = new ControlFlowEngine<Schemes>(({ inputs, outputs }) => {
    return {
      inputs: () =>
        Object.entries(inputs)
          .filter(([_, input]) => input?.socket?.name === "Trigger")
          .map(([name]) => name),
      outputs: () =>
        Object.entries(outputs)
          .filter(([_, input]) => input?.socket?.name === "Trigger")
          .map(([name]) => name),
    };
  });
  return engine;
};
Ni55aN commented 1 year ago

but this one doesn't have TS generic parameter

Necmttn commented 1 year ago

but this one doesn't have TS generic parameter

You're right sorry, wrong branch. Updated.

Sure it's just a wrapper;

export const createControlFlowEngine = <
  Schemes extends ControlFlowEngineScheme
>() => {
  const engine = new ControlFlowEngine<Schemes>(({ inputs, outputs }) => {
    return {
      inputs: () =>
        Object.entries(inputs)
          .filter(([_, input]) => input?.socket?.name === "Trigger")
          .map(([name]) => name),
      outputs: () =>
        Object.entries(outputs)
          .filter(([_, input]) => input?.socket?.name === "Trigger")
          .map(([name]) => name),
    };
  });
  return engine;
};
Ni55aN commented 1 year ago

what is ControlFlowEngineScheme ?

Necmttn commented 1 year ago

what is ControlFlowEngineScheme ?

it's from the rete-engine

export declare type ControlFlowEngineScheme = GetSchemes<ClassicScheme['Node'] & {
    execute(input: string, forward: (output: string) => void): void;
}, ClassicScheme['Connection']>;
Ni55aN commented 1 year ago

Ok, thanks, I'll check it

Ni55aN commented 1 year ago

can you share a reproducible example?

I've tried it in the default template:

npx rete-kit app -n issue-668 -s react-vite -v 18 -f 'react render,dataflow engine'
const createControlFlowEngine = <
  Schemes extends ControlFlowEngineScheme
>() => {
  const engine = new ControlFlowEngine<Schemes>(({ inputs, outputs }) => {
    return {
      inputs: () =>
        Object.entries(inputs)
          .filter(([_, input]) => input?.socket?.name === "Trigger")
          .map(([name]) => name),
      outputs: () =>
        Object.entries(outputs)
          .filter(([_, input]) => input?.socket?.name === "Trigger")
          .map(([name]) => name),
    };
  });
  return engine;
};

const engine = createControlFlowEngine<Schemes>()

// additionally paste this inside NumberNode and AddNode
execute() {}

Even if I explicitly specify Input/Output keys

class NumberNode extends Classic.Node<{}, { value: Classic.Socket }> implements DataflowNode {
}
///
class AddNode extends Classic.Node<{ a: Classic.Socket, b: Classic.Socket }> implements DataflowNode {
}

it doesn't show an error

Necmttn commented 1 year ago

Oh, it might be due to my extended Socket definition, maybe?

export abstract class BaseNode<
  Machine extends AnyStateMachine,
  Inputs extends {
    [key in string]?: AllSockets;
  } = {
    [key in string]?: AllSockets;
  },
  Outputs extends {
    [key in string]?: AllSockets;
  } = {
    [key in string]?: AllSockets;
  },
  Controls extends {
    [key in string]?: BaseControl & { name?: string };
  } = {
    [key in string]?: BaseControl & { name?: string };
  }
> extends ClassicPreset.Node<Inputs, Outputs, Controls> {

Which i took inspiration from one of the examples;

class DatabaseIdSocket extends Socket {
  constructor() {
    super("databaseIdSocket");
  }
}
export const databaseIdSocket = new DatabaseIdSocket();

databaseIdSocket.combineWith(stringSocket);

const sockets = [
  numberSocket,
  booleanSocket,
  stringSocket,
  arraySocket,
  objectSocket,
  eventSocket,
  audioSocket,
  documentSocket,
  embeddingSocket,
  taskSocket,
  imageSocket,
  databaseIdSocket,
] as const;

export type AllSockets = (typeof sockets)[number];
Ni55aN commented 1 year ago

still nothing

type AnyStateMachine = number

class DatabaseIdSocket extends Classic.Socket {
  constructor() {
    super("databaseIdSocket");
  }
}
export const databaseIdSocket = new DatabaseIdSocket();

const sockets = [
  databaseIdSocket,
] as const;

export type AllSockets = (typeof sockets)[number];

export abstract class BaseNode<
  Machine extends AnyStateMachine,
  Inputs extends {
    [key in string]?: AllSockets;
  } = {
    [key in string]?: AllSockets;
  },
  Outputs extends {
    [key in string]?: AllSockets;
  } = {
    [key in string]?: AllSockets;
  },
  Controls extends {
    [key in string]?: Classic.Control & { name?: string };
  } = {
    [key in string]?: Classic.Control & { name?: string };
  }
> extends Classic.Node<Inputs, Outputs, Controls> {
  m?: Machine
}

type Node = NumberNode | AddNode;
type Conn =
  | Connection<NumberNode, AddNode>
  | Connection<AddNode, AddNode>
  | Connection<AddNode, NumberNode>;
type Schemes = GetSchemes<Node, Conn>;

const engine = createControlFlowEngine<Schemes>()

What's your TS version?