Warning: Maximum update depth exceeded on basic example

[bfig]

Hey man, great lib in general, some cool features here!

I'm trying to get the basics right and I'm having trouble with the editor behaving in this unusual way. As soon as I connect any two nodes even with the basic example here: https://flume.dev/docs/basic-config, I start getting this error non stop:

react_devtools_backend.js:4026 Warning: Maximum update depth exceeded. This can happen when a component calls setState inside useEffect, but useEffect either doesn't have a dependency array, or one of the dependencies changes on every render. at NodeEditor (http://localhost:3000/static/js/bundle.js:10660:30) at div at App

I've compiled it by running the latest create react app, this is how my package.json looks like:

  "packages": {
    "": { 
      "name": "cpq-actionprocess",
      "version": "0.1.0",
      "dependencies": {
        "@testing-library/jest-dom": "^5.16.4",
        "@testing-library/react": "^13.2.0",
        "@testing-library/user-event": "^13.5.0",
        "flume": "^0.8.0",
        "react": "^18.1.0",
        "react-dom": "^18.1.0",
        "react-scripts": "5.0.1",
        "web-vitals": "^2.1.4"
      }     
    },    

Hope this is useful.

[chrisjpatty]

Thanks @bfig, do you have a repo I could look at to try to recreate this?

[bfig]

https://github.com/tdtconsultants/cpq-flumetest thanks for the quick response!

[mwolfeu]

Same issue here.

Simple example: Fill in the csv node path text input. Attach its output to a log root node. The path string will print out in the console.

It works, but with an unending barrage of console warnings.

Config

const config = new FlumeConfig();

// Colors from Flume - They should be expanding soon
Colors = {
  data: "orange",  // datasets, & geoJSON until we have more colors
  layer: "pink", // compositable layer
  scale: "yellow",
  // purple: "purple",
  // red: "red",
  // blue: "blue",
  // green: "green",
  // grey: "grey",
};

// config util generating: type, label, name
let cfgTLN = (d) => {
  safe = d.toLowerCase().replaceAll(' ', '_');
  return {type: safe, name: safe, label:d}
  };

// config util generating: type, label
let cfgTL = (d) => {
  safe = d.toLowerCase().replaceAll(' ', '_');
  return {type: safe, label:d}
  };

// List of all core node port types.
const portList = [
  {
    ...cfgTLN("Path"),
    hidePort: true,
    controls: [
      Controls.text({
        name: "string",
        label: "Path"
      })
    ]
  },
  {
    ...cfgTLN("JSON Dataset"),
    color: Colors.data,
  },
  {
    ...cfgTLN("Debug Data"),
    color: Colors.data,
    acceptTypes: ["json_dataset"]
  },
];

// List of all core node types.
const nodeList = [
  {
    ...cfgTL("CSV"),
    description: "CSV Loader -> JSON",
    inputs: ports => [
      ports.path()
    ],
    outputs: ports => [
      ports.json_dataset()
    ]
  },
  {   
    root:true,
    ...cfgTL("Log"),
    description: "Console.log",
    inputs: ports => [
      ports.debug_data()
    ],
  }
];

// custom nodes should be able to augment lists
portList.map(pt => config.addPortType(pt));
nodeList.map(nt => config.addNodeType(nt));

export default config;

Resolve Ports / Nodes

const resolvePorts = (portType, data, context) => {
  console.log("resolvePorts", portType, data, context)
  switch (portType) {
    case 'path':
      return data.string
    // case 'boolean':
    //   return data.boolean
    // case 'number':
    //   return data.number
    default:
      return data
  }
}

const resolveNodes = (node, inputValues, nodeType, context) => {
  console.log("resolveNodes", node, inputValues, nodeType, context);
  switch (node.type) {
    case 'csv':
      return { json_dataset: inputValues.path }
    case 'log':
      console.log('log', inputValues.debug_data)
      return { }
    // case 'boolean':
    //   return { boolean: inputValues.boolean }
    // case 'number':
    //   return { number: inputValues.number }
    // case 'user':
    //   return context.user
    // case 'joinText':
    //   return { joinedText: inputValues.string1 + inputValues.string2 }
    // case "reverseBoolean":
    //   return { boolean: !inputValues.boolean }
    default:
      return inputValues
  }
}

[s-jakob]

Any updates on that? I have the same issue here.

[elijah-potter]

Ditto

Any updates on that? I have the same issue here.

[staminna]

Same, with Node 18.4.0. Had to use Yarn instead of NPM to compile. Used create-react-app but compiles 100% no warnings except after creating two basic nodes and connect them into each other, from basic example on README

[elijah-potter]

It seems like this problem began in v0.7.1. I tried running the minimum with v0.6.3 and encountered no issues.

[mcpar-land]

Getting this error with the simplest possible example from the Getting Started guide in the documentation.

import React from "react";
import { NodeEditor, FlumeConfig, Colors, Controls } from "flume";
import "./App.css";

const config = new FlumeConfig();
config
    .addPortType({
        type: "string",
        name: "string",
        label: "Text",
        color: Colors.gray,
        controls: [
            Controls.text({
                name: "string",
                label: "Text",
            }),
        ],
    })
    .addNodeType({
        type: "string",
        label: "Text",
        description: "Outputs a string of text",
        inputs: (ports: any) => [ports.string()],
        outputs: (ports: any) => [ports.string()],
    });

function App() {
    return (
        <div className="App">
            <NodeEditor portTypes={config.portTypes} nodeTypes={config.nodeTypes} />
        </div>
    );
}

export default App;

Create two nodes, connect them via one wire, and the console will spit this error out multiple times per second.

react_devtools_backend.js:4026 Warning: Maximum update depth exceeded. This can happen when a component calls setState inside useEffect, but useEffect either doesn't have a dependency array, or one of the dependencies changes on every render.

Using Yarn, create-react-app, and React v18 and Flume v0.8

[pascallapradebrite4]

I believe I found the high-level cause and root cause of the issue.

High-level cause

The generic cause is this: Flume doesn't support out-of-the-box React 18's new concurrent mode (and it doesn't advertise it does, as it has React 17 in its peer dependencies, not React 18). Using the minimal example from the previous post, I could alternate between the sample crashing or not depending wether I used ReactDom.render() (legacy mode even in React 18, does not trigger the crash) and ReactDom.createRoot().render() (concurrent mode, triggers the crash).

As such, there is a first workaround for users currently experiencing this issue: if at all possible, switch back to using legacy root creation, which will disable concurrent mode and prevent the issue from happening. You can find the API doc here: https://reactjs.org/docs/react-dom.html#render If/when the root cause is fixed in Flume, you'll be able to go back to the current "concurrent" mode.

Root cause

The issue is caused in the NodeEditor component, having a LayoutEffect hook depending on a state variable (shouldRecalculateConnections), and then setting a new value for that state, combined with having a "manual" trigger function also being called in an effect deeper in the render tree (IoPorts.js/Input). I can't wrap my head around a precise, step-by-step trace of exactly what happens, but it seems to me like having an effect depending on a state it also sets, and having that same state set to something else in another render, provokes this.

You can find the concerned code here: https://github.com/chrisjpatty/flume/blob/0372e92e65065772234ab83c2a106da43279915a/src/index.js#L97:L106

However, since we know the state dependency is what causes the infinite recursion, we can remove or replace that state to prevent the issue from happening.

Fix

Note: I don't believe any of the two suggestions below would break on React < 18, which is important while the official peer dependency is React 17.

Depending on the intended behavior (which I couldn't conclude on my own based on the code), I see two options:

Option 1: completely remove the shouldRecalculateConnections state

This is the option I believe is less likely to be the intended behavior.

One option is to remove the shouldRecalculateConnections state, and simply call recalculateConnections() every time we want, instead of depending on an effect to do that calculation.

The resulting code is as follows:

React.useLayoutEffect(() => {
  recalculateConnections();
}, [recalculateConnections]);

const triggerRecalculation = () => {
  recalculateConnections();
};

When tracing calls locally, triggerRecalculation works like before, since it previously setting the state resulted in a recalculation. However, the layout effect now calls recalculateConnections more often, since it doesn't have a quick break when the state var would previously bypass the calculation. It seems to be called every time a node changes, so not that often, but still.

Option 2: replace the state with an "initial did X" ref

This is the option I believe closest matches the current behavior.

The other option is based on the fact that only triggerRecalculation ever sets shouldRecalculateConnections to true, thus causing a recalculation in the effect hook. I also see that the initial value of shouldRecalculateConnections is true, which means that on initial render, the recalculation will also be called.

If the intended behavior is "recalculate on first render, then on every trigger", then a ref var can be used instead of state to accomplish this, as follows:

const initialRecalculateConnectionsDoneRef = React.useRef(false);

React.useLayoutEffect(() => {
  if (!initialRecalculateConnectionsDoneRef.current) {
    // We want a single recalculation on the first render,
    // then subsequent recalculations should happen on triggers instead.
    initialRecalculateConnectionsDoneRef.current = true;
    recalculateConnections();
  }
}, [recalculateConnections]);

const triggerRecalculation = () => {
  recalculateConnections();
};

I tested both options, and both prevented the crash from happening.

Of course, my knowledge of the code base and its intricacies is limited, and I could fail to see an issue with the suggestions above, but from my perspective, it seems to work as the original.

Workarounds

Any one of these can fix the issue:

Switch back to non-concurrent React

As I mentioned above, one workaround is to opt-out of React's current "concurrent" mode, by switching to ReactDom.render() instead of ReactDom.createRoot().render().

Use patch-package

For users of patch-package, I also wrote the following patch, which applies "Option 2", if you believe it is sufficient for your needs:

diff --git a/node_modules/flume/dist/index.es.js b/node_modules/flume/dist/index.es.js
index bbd3262..2f0a9b0 100644
--- a/node_modules/flume/dist/index.es.js
+++ b/node_modules/flume/dist/index.es.js
@@ -7751,15 +7751,17 @@ var NodeEditor = function NodeEditor(_ref, ref) {
     stage.current = document.getElementById("" + STAGE_ID + editorId).getBoundingClientRect();
   };

+  var initialRecalculateConnectionsDoneRef = React.useRef(false);
+
   React.useLayoutEffect(function () {
-    if (shouldRecalculateConnections) {
+    if (!initialRecalculateConnectionsDoneRef.current) {
+      initialRecalculateConnectionsDoneRef.current = true;
       recalculateConnections();
-      setShouldRecalculateConnections(false);
     }
-  }, [shouldRecalculateConnections, recalculateConnections]);
+  }, [recalculateConnections]);

   var triggerRecalculation = function triggerRecalculation() {
-    setShouldRecalculateConnections(true);
+    recalculateConnections();
   };

   React.useImperativeHandle(ref, function () {
diff --git a/node_modules/flume/dist/index.js b/node_modules/flume/dist/index.js
index 47d32c4..843f50d 100644
--- a/node_modules/flume/dist/index.js
+++ b/node_modules/flume/dist/index.js
@@ -7758,15 +7758,17 @@ exports.NodeEditor = function NodeEditor(_ref, ref) {
     stage.current = document.getElementById("" + STAGE_ID + editorId).getBoundingClientRect();
   };

+  var initialRecalculateConnectionsDoneRef = React__default.useRef(false);
+
   React__default.useLayoutEffect(function () {
-    if (shouldRecalculateConnections) {
+    if (!initialRecalculateConnectionsDoneRef.current) {
+      initialRecalculateConnectionsDoneRef.current = true;
       recalculateConnections();
-      setShouldRecalculateConnections(false);
     }
-  }, [shouldRecalculateConnections, recalculateConnections]);
+  }, [recalculateConnections]);

   var triggerRecalculation = function triggerRecalculation() {
-    setShouldRecalculateConnections(true);
+    recalculateConnections();
   };

   React__default.useImperativeHandle(ref, function () {

The patch fixed the issue locally.

Final notes

If the library's owner accepts pull requests, I will be happy to open one with the fix in option 2 (or something else if you have comments). As the fix is quite simple, you might also prefer to make the changes yourself.

And, thanks, by the way, for this awesome library 🙂

[remkop22]

@pascallapradebrite4

If the library's owner accepts pull requests, I will be happy to open one with the fix in option 2 (or something else if you have comments). As the fix is quite simple, you might also prefer to make the changes yourself.

As a consumer of this library I would appreciate a PR with that fix a great deal! 😄

[JustinB-AB]

Same issue, tried to make it work, but couldn't and have to move on. Too bad, it's a very easy to use package, just now out of date. Will subscribe in case there's a fix made in the future.

[ghalex]

Same issue here, sad you can't use it with react 18

[danlobo]

It looks like the recursion takes place in src/components/IoPorts.js. I've logged all the effects in this file and it looks like the recursion takes place inside the useEffect on lines 119-123.

The variable isConnected is false and prevConnected is true when the function is called based on dependencies. I don't know what this code does, but removing it doesn't seem to cause any collateral damage, at least in the tests I've done.

I'll do more tests tomorrow, but @chrisjpatty , can you help me figuring out what this code snipped does?

[atwright147]

@pascallapradebrite4 I am using your patch, but please raise a PR 😁

@chrisjpatty please fix or merge this 😁. This is breaking your wonderful project

[osafafi]

@chrisjpatty As you can see, we are eager to use your wonderful tool. however this issue is blocking me from moving forward and i think you have the best and cleanest nodebased editor on the market. Are there plans to upgrade to React 18 and fix this issue? cheers!

[chrisjpatty]

@osafafi I'm looking into it right now. Having some trouble reproducing but I should have a patch soon hopefully.

[chrisjpatty]

This should be fixed now in version 0.8.1. Thanks for everyone's patience on this, and thanks especially @pascallapradebrite4 for the great writeup, was very helpful!

[osafafi]

@chrisjpatty wonderful it's working like a charm now ! Cheers @pascallapradebrite4 ! gotta love it when things run smoothly !! Bravo lads