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

Improvement - separate node state from node definition. #167

Open
oveddan opened this issue Nov 24, 2022 · 0 comments
Open

Improvement - separate node state from node definition. #167

oveddan opened this issue Nov 24, 2022 · 0 comments

Comments

@oveddan
Copy link
Collaborator

oveddan commented Nov 24, 2022

As discussed in the discord, it would be great to separate node state from its definition, and provide an easier/simpler api for implementing nodes.

Currently you need to declare a node as a class that inherits from a node type, and create a constructor that passes values to the parent, for example with Branch:

export class Branch extends FlowNode {
  public static Description = new NodeDescription(
    'flow/branch',
    'Flow',
    'Branch',
    (description, graph) => new Branch(description, graph)
  );

  constructor(description: NodeDescription, graph: Graph) {
    super(
      description,
      graph,
      [new Socket('flow', 'flow'), new Socket('boolean', 'condition')],
      [new Socket('flow', 'true'), new Socket('flow', 'false')]
    );
  }

  triggered(fiber: Fiber, triggeringSocketName: string) {
    fiber.commit(
      this,
      this.readInput<boolean>('condition') === true ? 'true' : 'false'
    );
  }
}

A few issues with this approach:

  • It inherits from two different classes - you have to follow values/methods up the inheritance tree
  • A lot of boilerplate code to define a node
  • State is stored in a bunch of nodes and not all in one place and can be harder to debug.
  • State is combined with node definition
  • You have access to the whole fiber object, but in reality you don't and shouldn't have access to its entire api. exposing more of the api than is needed makes things harder to implement and more prone to calling it the wrong way.

A much easier thing to implement method could look like:

interface INode {
     typeName: string,
     category: NodeCategory,     
     label: string,
     inputs: Socket[],
     outputs: Socket[]
}

interface IFlowNode extends INode {
    triggered: (
      writeOutput: (param: string, value: any) => void,
      readInput: <T>(param: string) => T,
      commit: (param: string) => void,
      triggeringSocketName: string
    ) => void;
}

Then the implementation is as clean/readable as:

export const Branch: IFlowNode = {
  typeName: 'flow/branch',
  category: 'Flow',
  label: 'Branch',
  inputs: [new Socket('flow', 'flow'), new Socket('boolean', 'condition')],
  outputs: [new Socket('flow', 'true'), new Socket('flow', 'false')],
  triggered: (_, readInput, commit) => {
    commit(readInput<boolean>('condition') === true ? 'true' : 'false');
}

This also is more testable - it's easier to write tests around this triggered function this way as you can simulate state and don't need to setup the entire engine/fiber classes.

If all the nodes state is stored in one place, lets say in the engine class, its easier to debug; it also makes it possible to reset the entire nodes state without having to rebuilt all these definition instances.

This is also more readable - you can know what variables flow/branch Flow and Branch represent by just looking at the code, without having to go to the definition of NodeDescription constructor

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

No branches or pull requests

1 participant