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

Revert "Fix input output execution" #277

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

SorsOps
Copy link
Member

@SorsOps SorsOps commented Jun 17, 2024

User description

Reverts #276


PR Type

Bug fix


Description

  • Fixed the logic for displaying values and port names in the node wrapper component.
  • Simplified the execute method in both input and output nodes by clearing and resetting outputs.
  • Removed unnecessary imports and the getAllOutputs method in the node programmatic file.

Changes walkthrough 📝

Relevant files
Bug fix
nodeV2.tsx
Fix value and port name display logic in node wrapper.     

packages/graph-editor/src/components/flow/wrapper/nodeV2.tsx

  • Changed condition to check for falsy values instead of undefined.
  • Removed redundant case for string type in getValuePreview.
  • Updated logic for displaying port names.
  • +4/-6     
    input.ts
    Simplify execute method and clean up imports in input node.

    packages/graph-engine/src/nodes/generic/input.ts

  • Removed unnecessary imports.
  • Simplified the execute method to clear and reset outputs.
  • +10/-17 
    output.ts
    Simplify execute method and clean up imports in output node.

    packages/graph-engine/src/nodes/generic/output.ts

  • Removed unnecessary imports.
  • Simplified the execute method to clear and reset outputs.
  • +10/-17 
    node.ts
    Remove getAllOutputs method and reorganize imports in node.

    packages/graph-engine/src/programmatic/node.ts

  • Reorganized imports for better readability.
  • Removed the getAllOutputs method.
  • +5/-11   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    changeset-bot bot commented Jun 17, 2024

    ⚠️ No Changeset found

    Latest commit: 8e3425b

    Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

    This PR includes no changesets

    When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

    Click here to learn what changesets are, and how to add one.

    Click here if you're a maintainer who wants to add a changeset to this PR

    @SorsOps SorsOps merged commit 0cca777 into major/strong-typing Jun 17, 2024
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 3
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    The change from checking value === undefined to !value in getValuePreview might introduce bugs by treating all falsy values (like 0 or empty string) as undefined, which could lead to incorrect behavior or display issues.
    Code Clarity:
    The removal of the getAllOutputs method and changes in the execute methods of input.ts and output.ts simplify the code but could potentially impact functionality if the original handling of outputs was necessary for certain edge cases.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Use strict equality check to avoid incorrect handling of falsy values

    Replace the loose check (!value) with a strict check (value === undefined) to ensure that
    only undefined values return null. This avoids unintended cases where falsy values like 0
    or '' would incorrectly return null.

    packages/graph-editor/src/components/flow/wrapper/nodeV2.tsx [166-167]

    -if (!value) {
    +if (value === undefined) {
       return null;
     }
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential bug where falsy values like 0 or '' could incorrectly return null. Using a strict check ensures only undefined values are handled this way, improving the function's reliability.

    9
    Enhancement
    Prevent output duplication by checking for existing outputs before adding new ones

    Reintroduce the check for existing outputs before adding new outputs to prevent
    duplication and potential overwrites in the node's output data.

    packages/graph-engine/src/nodes/generic/input.ts [34-38]

    -this.addOutput(input, {
    -  type: rawInput.type,
    -  visible: true,
    -});
    +if(!(input in this.getAllOutputs())) {
    +  this.addOutput(input, {
    +    type: rawInput.type,
    +    visible: true,
    +  });
    +}
     this.setOutput(input, rawInput.value);
     
    Suggestion importance[1-10]: 8

    Why: This suggestion enhances the code by preventing potential duplication and overwrites in the node's output data, which can improve the stability and correctness of the node's behavior.

    8
    Maintainability
    Restore the getAllOutputs method to manage node outputs effectively

    Reintroduce the getAllOutputs method to facilitate access to all outputs, which might be
    necessary for other parts of the application or during node cleanup.

    packages/graph-engine/src/programmatic/node.ts [285-287]

    +getAllOutputs = <T = Record<string, any>>(): T => {
    +  return Object.fromEntries(
    +    Object.entries(this.outputs).map(([key, value]) => [key, value.value])
    +  ) as T;
    +};
    +
     /**
      * Handles cleanup for nodes with state.
      * Use the super method to clear the graph reference
     
    Suggestion importance[1-10]: 8

    Why: Reintroducing the getAllOutputs method improves maintainability and ensures that other parts of the application or node cleanup processes can access all outputs as needed.

    8
    Possible issue
    Ensure string values are handled correctly if needed

    Since the 'string' case handling has been removed, ensure that this does not affect the
    functionality where string values need specific handling other than being checked as a hex
    color.

    packages/graph-editor/src/components/flow/wrapper/nodeV2.tsx [192-194]

    +case 'string':
    +  valuePreview = value;
    +  break;
     default:
       if (isHexColor(value)) {
         return getColorPreview(value,true);
     
    Suggestion importance[1-10]: 7

    Why: This suggestion ensures that string values are handled correctly, which might be necessary for certain functionalities. However, it is context-dependent and might not be crucial if string handling as hex color is sufficient.

    7

    SorsOps added a commit that referenced this pull request Jun 18, 2024
    * Stash
    
    * Stash
    
    * WIP
    
    * Cleanup inline nodes and re-enable array nodes
    
    * Add drop panel to show windows
    
    * Remove settings dialog
    
    * Fix bug with values sticking if selecting between two nodes of the same type
    
    * Fix not reinvalidating the node settings
    
    * Fix issue with duplication
    
    * Fix audio source
    
    * Update UI package and simplify dialog
    
    * Cleanup file explorer
    
    * Update files system
    
    * Add support for audio buffer conversions
    
    * Fix changed bufferschema refs in audio pkg
    
    * add ceil and floor nodes
    
    * Subgraphs mostly working
    
    * Cleanup not selecting the correct graph on tab transition
    
    * Cleanup typescript types
    
    * Cleanup UI and fix deserialization of edges
    
    * Additional UI cleanup
    
    * Fix padding on settings
    
    * Minor update on documentation
    
    * Cleanup on styles
    
    * Fix nodenext resolution
    
    * Cleanup ts config
    
    * Cleanup ts import for consistency
    
    * Replace all .ts? imports
    
    * Fix issue with typing
    
    * Add assets for apple touch startup
    
    * Add initial k8s for mailcatcher for local dev
    
    * Add docker-compose for open telemetry for nextjs
    
    * Cleanup styles
    
    * Add postgres for persistence
    
    * Fix circular import
    
    * Fix menu Item boolean value issue
    
    * Remove console log
    
    * Add prometheus and otel config
    
    * Set default to be darktheme
    
    * Cleanup menubar rest render spread
    
    * Move controls for token arrays to the token package
    
    * Cleanup old redux models
    
    * Remove global css usage
    
    * Set darktheme to be the default and remove global css usage
    
    * Remove old output providers from preview. They will need to be reworked to handle routing the data
    
    * Add support for client side storage providers
    
    * Add mailcatcher and postgres to docker-compose
    
    * Add otel instrumentation
    
    * Update packge for otel
    
    * Setup ory kratos, replace mailcatcher with mailslurper
    
    * Add tracing for debugging
    
    * Restyle node (#245)
    
    * restyle node
    
    * restyle node
    
    * restyle node
    
    * remove unused background
    
    * remove unused background
    
    * remove string from import
    
    * fix pointer events on handle
    
    * Fix storybook setup
    
    * add rail
    
    * style graph editor
    
    * update node style to have more contrast
    
    * add exp and sqrt node
    
    * Add initial scaffold for backend
    
    * change icon for type any
    
    * mute down handle background
    
    * fix transparent background on panels
    
    * Fix graph loading
    
    * Swap ui to use scss and remove unnecessary styling
    
    * Add scss support for Marco and fix non .js imports
    
    * fix reference to styles.css to .scss
    
    * Add example of barebones implementation of the graph-editor
    
    * Cleanup file
    
    * Fix issue with zustand not being the same
    
    * Extend color ranges and fix colors showing up correctly in port previews
    
    * Remove console log during graph serialization
    
    * Update backend to use prisma, add in DI, setup ory kratos connection
    
    * Add oathkeeper and kratos setup
    
    * Add next link for rail
    
    * Remove drag initiator styling
    
    * Add generated SDK
    
    * Update backend package
    
    * Remove copied changelog
    
    * Get backend docker build working
    
    * Move docker resources and add a readme entry for running the docker compose
    
    * Remove all icon packages except for iconoir
    
    * Remove import assertion for importing json.  We assume this is unnecessary
    
    * Remove DB, we will assume migration as needed
    
    * Replace graphlib with later version
    
    * Ignore users local db
    
    * Move docker compose env to root
    
    * Graphlib replace
    
    * Optimize icon usage
    
    * Add basic keto client, setup proper documentation
    
    * Add initial controllers
    
    * Update SDK
    
    * Add SDK to UI
    
    * Cleanup docs
    
    * Update edge docs
    
    * Fix oathkeeper origins
    
    * Expand models
    
    * Ignore noisy generated changes
    
    * Ignore noisy files
    
    * Export typing on the serialized graph
    
    * Clean cors
    
    * Remove cors as expected to be handled externally
    
    * Update kratos
    
    * Fix ref
    
    * Untrack generated
    
    * ignore json import which should be external
    
    * Persist dynamic type
    
    * Integrate API
    
    * Remove console log
    
    * Remove prometheus
    
    * Add example for preview
    
    * add preview nodes
    
    * Force generation on build
    
    * add viewport saving, math expression visualisation, default values for number
    
    * add gradient stops and gradient handle positions
    
    * Setup storybook test
    
    * remove gradient handles, add gradient schema
    
    * Export util functions
    
    * Don't require typing on input unless its intentionally dynamic
    
    * Add storybook example
    
    * Add delayed updates to controls, fix issue with choosing a type and not having the control update
    
    * Fix non standard curve change that was breaking preview
    
    * Fix issue with token set controls and expose them correctly
    
    * Fix token saving not serializing correctly
    
    * Remove live update of position annotation, was causing unnecessary recalc
    
    * Clean up hot key usage and first pass at fixing subgraphs
    
    * Add manifest for PWA in the future
    
    * Cleanup UI and start app router migration
    
    * Clean up docs
    
    * Mention initial db setup for prisma
    
    * Setup for gitpod
    
    * Cleanup build type errors
    
    * More gitpod setup
    
    * Additional setup for app router conversion
    
    * Fix for Marco
    
    * Fix style ordering
    
    * Update graph-engine to use tsup over rollup. Remove aliasing as the esbuild does not seem to respect the alias transform
    
    * Finish converting other node packages to use tsup
    
    * Fix missing tsx for tsup
    
    * Fix issue with token preview if undefined
    
    * Fix menu bombing due to nextjs removing svg loader
    
    * Remove old rollup file
    
    * Continued rollup removal
    
    * add new colors for types on handles
    
    * change size of handle icon
    
    * Update tsup configs
    
    * Fix type problem with engine bundling
    
    * Fix build splitting
    
    * Cleanup hotkeys
    
    * Fix external sanitize usage
    
    * add radix ui colors
    
    * Cleanup docker build
    
    * Update dep for design-tokens
    
    * Fix host name on deploy
    
    * Fix for readiness probe
    
    * Stop redirecting on index
    
    * Remove second level domain
    
    * Revert
    
    * Setup lit example of context usage
    
    * Add typograph preview specifics
    
    * Add building the backend and docs to workflow
    
    * Make the docs use the same dev command as everything else
    
    * Fix variadic typing for Marco
    
    * Change build command for documentation
    
    * Allow dynamic inputs for array map subgraph
    
    * Fix issue with subgraph typing for tokens
    
    * Minor fix to display the font size
    
    * Add cypress testing to graph-editor
    
    * Deploy graph docs
    
    * Re add up to date position changes
    
    * Add basic vector 2 support
    
    * Simplify typing on arrays
    
    * Cleanup from changes
    
    * Add setting to hide minimap and to show the title in the subgraph
    
    * Fix finding nodes
    
    * Improve searching
    
    * Add copy value to port
    
    * Stop subgraph from gathering input values every loop
    
    * Add reduce subgraph
    
    * Expose convert color
    
    * Fix poline
    
    * Make series consistent with naming
    
    * Add advanced blending to design tokens
    
    * Cleanup schemas on token arrays
    
    * Fix cases where input is not detected
    
    * Add secret and deployment
    
    * Add alignment and distribution
    
    * Add better support for network messages
    
    * Fix bug where we don't disconnect an edge and minor cleanup in tests
    
    * Complete subgraph creation , convert output to multi output
    
    * Cleanup typing on graph
    
    * Update docs
    
    * Update docs
    
    * Fix issue with subgraph not reconnecting nodes
    
    * Fix grouping
    
    * Start releasing on dockerhub
    
    * Remove tauri
    
    * Update versions
    
    * Fix ignore
    
    * Update Gimlet
    
    * Update deployment for docs and backend
    
    * fix bugs
    
    * rm log
    
    * fix input inputs
    
    * add dimension
    
    * 250: fix shift+k (#251)
    
    * fix(250): Fix quick search triggered in input fields
    Co-authored-by: SorsOps <80043879+sorsOps@users.noreply.github.com>
    
    * Reorder variadic ports (#258)
    
    
    
    ---------
    
    Co-authored-by: SorsOps <80043879+sorsOps@users.noreply.github.com>
    
    * fix color wheel (#261)
    
    * Show value preview in nodes (#257)
    
    * Show value preview in nodes
    
    * Add dot for variadic and other improvements
    
    * Fix edge case with object variadics and allow toggling on and off of inline values
    
    * Merge george's changes
    
    ---------
    
    Co-authored-by: SorsOps <80043879+sorsOps@users.noreply.github.com>
    
    * fix valid check (#263)
    
    * Feat/debugger (#259)
    
    * Add debugger and remove flamegraph
    
    * add xyz65 to color convert
    
    * fix flatten node
    
    * add flatten alpha node
    
    * fix(1013): Subgraph ouput
    
    * Setup alpha and beta deploys (#270)
    
    * Hide unfinished work (#271)
    
    * Setup alpha and beta deploys
    
    * Hide unfinished work
    
    * Fix arrayMap and arrayReduce
    
    * fix(273): Fix input output execution
    
    * Copy type to outputs
    
    * fix(273): Fix input output execution (#274)
    
    * fix(273): Fix input output execution
    
    * Copy type to outputs
    
    * Revert "fix(273): Fix input output execution (#274)" (#275)
    
    This reverts commit 6d14dbc.
    
    * Revert "Fix input output execution" (#277)
    
    * feat: Update input.ts to handle dynamic type changes in Input.setValue()
    
    * Feat/documentation (#279)
    
    * Add script to generate documentation
    Remove 'NodeTypes` in favor of direct values for the node types
    Change input/output nodes to not clear and instead diff values
    Fixed some issues with flexbox of the node wrapper
    Add in an array indicator
    Fixed subgraph to be reactive to additional outputs
    Fix find menu
    
    * add contrast alpha node (#272)
    
    * add contrast alpha node
    
    * Remove console logs
    
    ---------
    
    Co-authored-by: SorsOps <80043879+sorsOps@users.noreply.github.com>
    
    * UI rework (#278)
    
    * add subtitle and style ui
    
    * Fix select and download
    
    * Add constant for graph id
    
    ---------
    
    Co-authored-by: Marco Christian Krenn <marco.krenn@gmail.com>
    
    * prevent delete when annotation is present (#281)
    
    * Fix inputs which are any type to be overridden with new values. (#282)
    
    Fix typing with the array subgraph
    Allow creating array input types
    
    * add colors to legend (#283)
    
    * Feat/token sets (#284)
    
    * Fix inputs which are any type to be overridden with new values.
    Fix typing with the array subgraph
    Allow creating array input types
    
    * Expose additional vars in the array map and add the ability to convert to a token set
    
    * Fix/command k (#287)
    
    * Fix inputs which are any type to be overridden with new values.
    Fix typing with the array subgraph
    Allow creating array input types
    
    * Expose additional vars in the array map and add the ability to convert to a token set
    
    * Fix command palette messing up positions
    
    * Fix contrasting alpha import
    
    * Add markdown support
    
    * Change objectmerge and flatten to not  be variadic
    
    * fix: input bug
    
    * Remove console logs
    
    * Remove console logs
    
    * Fix color wheel (#300)
    
    * fix color wheel calculation
    
    * add changeset
    
    * fix position reference (#298)
    
    * fix pos reference
    
    * spread annotations
    
    * Feat/prep master (#303)
    
    * Fix can delete.
    Fix issues with variadic passing through type for any array.
    Prep for the major overhaul
    Add back in examples
    
    * Setup gimlet for deploys
    
    ---------
    
    Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
    Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
    Co-authored-by: popdrazvan <popdrazvan@gmail.com>
    Co-authored-by: Vaibhav Bhosale <mail2vsbhosale@gmail.com>
    Co-authored-by: Jan Six <six7@github.com>
    Co-authored-by: Jan Six <six.jan@gmail.com>
    
    ---------
    
    Co-authored-by: Marco Christian Krenn <marco.krenn@gmail.com>
    Co-authored-by: popdrazvan <popdrazvan@gmail.com>
    Co-authored-by: georgebuciuman <georgebuciuman@gmail.com>
    Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
    Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
    Co-authored-by: Vaibhav Bhosale <mail2vsbhosale@gmail.com>
    Co-authored-by: Jan Six <six7@github.com>
    Co-authored-by: Jan Six <six.jan@gmail.com>
    @SorsOps SorsOps deleted the revert-276-fix-input-output-execuion branch July 3, 2024 09:33
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant