Mobx Array when using push

[xiaofine1122]

I get this error when i use array.push

mobx.module.js:3138 Uncaught Error: [mobx] Modification exception: the internal structure of an observable array was changed.
at ObservableArrayAdministration.updateArrayLength (mobx.module.js:3138)
at ObservableArrayAdministration.spliceWithArray (mobx.module.js:3176)
at Proxy.push (mobx.module.js:3302)
at AppState.js:127

The Source code is

@observable gData = [{
    "title": "1",
    "key": "1",
    "children": [
      {
        "title": "2",
        "key": "2",
        "children": [
          {
            "title": "3",
            "key": "3"
          },
          {
            "title": "34",
            "key": "33"
          },
          {
            "title": "32",
            "key": "32"
          }
        ]
      },
      {
        "title": "21",
        "key": "21"
      }
    ]
  },

  {
    "title": "11",
    "key": "11"
  }];

onDrop = info => {
    console.log(info);
    const dropKey = info.node.props.eventKey; 
    const dragKey = info.dragNode.props.eventKey; 
    const dropPos = info.node.props.pos.split('-');
    const dropPosition = info.dropPosition - Number(dropPos[dropPos.length - 1]);

    const loop = (data, key, callback) => {
      data.forEach((item, index, arr) => {    // I think the cause of the problem***************
        if (item.key === key) {
          return callback(item, index, arr);
        }
        if (item.children) {
          return loop(item.children, key, callback);
        }
      });
    };
    
    const data = [...this.gData];
    // Find dragObject
    let dragObj;
	
    loop(data, dragKey, (item, index, arr) => {
      arr.splice(index, 1); 
      dragObj = item;
    });

    if (!info.dropToGap) {
      // Drop on the content
      loop(data, dropKey, item => {
        item.children = item.children || [];
        // where to insert     
        item.children.push(dragObj); // The Error location***************
      });
    } else if (
      (info.node.props.children || []).length > 0 && // Has children
      info.node.props.expanded && // Is expanded
      dropPosition === 1 // On the bottom gap
    ) {
      loop(data, dropKey, item => {
        item.children = item.children || [];
        // where to insert 
        item.children.unshift(dragObj);
      });
    } else {
      let ar;
      let i;
      loop(data, dropKey, (item, index, arr) => {
        ar = arr;
        i = index;
      });
      if (dropPosition === -1) {
        ar.splice(i, 0, dragObj);
      } else {
        ar.splice(i + 1, 0, dragObj);
      }
    }

    this.gData = data;
  };

I think the reason is ' data.forEach '. The item in the loop is not a proxy object ,but 'item.children' is a proxy object
So I replaced ‘data.forEach’ with the following function ,and no errors have occurred since

const myforEach = (arr,fn) =>{
      let i
      for(i=0; i<arr.length; i++){
        fn(arr[i],i,arr); 
      }  
    }

Is it a problem with foreach or am I using it wrong ?

the version i use is

"mobx": "^5.15.4",
"mobx-react": "^6.1.8",

[danielkcz]

Please, try to minimize that code to show an actual issue at hand or ideally setup CodeSandbox. It's unclear what problem should forEach cause.

[xiaofine1122]

The code is uploaded to
https://codesandbox.io/s/eager-kowalevski-k6mwd

But this error cannot be reproduced in codesandbox
Errors can be reproduced when downloading the source code to my computer，and opening http://localhost:3000/
My computer's system is windows10
The node version is v12.16.1.

[urugator]

You're probably iterating over an array and mutating it at the same time.
You can solve it by iterating over a copy or create new array and replace the original after the iteration.
For standard non observable array some operations might be safe (eg removing items inside forEach), but others will mess up the iteration/indices.
You can investigate in which scenarios we deviate from standard behavior and then we can take a look if there is something to do about it.
For reference https://www.bennadel.com/blog/2992-mutating-an-array-during-foreach-iteration-in-javascript.htm

[xiaofine1122]

You're probably iterating over an array and mutating it at the same time.
You can solve it by iterating over a copy or create new array and replace the original after the iteration.
For standard non observable array some operations might be safe (eg removing items inside forEach), but others will mess up the iteration/indices.
You can investigate in which scenarios we deviate from standard behavior and then we can take a look if there is something to do about it.
For reference https://www.bennadel.com/blog/2992-mutating-an-array-during-foreach-iteration-in-javascript.htm

Thank you for your reply

I do n’t think it ’s because of the indexing problem you mentioned.
After observing the data type, it is found that the data types of item and arr in the forEach loop are different, item is ‘proxy’, but ‘item’ is ‘array’.
Caused the use of 'arr.splice (index, 1);' did not use the splice method in mobx, did not change 'lastKnownLength', but I later 'item.children.push (dragObj);' and used mobx 'push 'Method, eventually led to errors.

Maybe can change the type of ‘arr’ in forEach to ‘proxy’ so that there will be no problem when operating

[urugator]

I see, but assuming that you cannot modify the array during iteration (due to the problems explained above), then it shouldn't matter that the third argument isn't the actual proxied array, because it's for reading only. I mean even if the arr would be a correct array, then mutating it is still a problem.

Isolated reproduction of possible issue:

const observableArray = observable([1,2,3]);
observableArray.forEach((item,index,arr) => {
  console.log(observableArray === arr); // false
});

Culprit

mobx/src/v5/types/observablearray.ts

Lines 474 to 475 in 8a63a29

	* Without this, everything works as well, but this works
	* faster as everything works on unproxied values

[xiaofine1122]

I see, but assuming that you cannot modify the array during iteration (due to the problems explained above), then it shouldn't matter that the third argument isn't the actual proxied array, because it's for reading only. I mean even if the arr would be a correct array, then mutating it is still a problem.

Isolated reproduction of possible issue:

const observableArray = observable([1,2,3]);
observableArray.forEach((item,index,arr) => {
  console.log(observableArray === arr); // false
});

Culprit

mobx/src/v5/types/observablearray.ts

Lines 474 to 475 in 8a63a29

	* Without this, everything works as well, but this works
	* faster as everything works on unproxied values

Thanks for your reply, I understand the problems in my previous understanding.
If I need to modify the array in the loop, I should use other looping methods such as ‘for’ loop, not “forEach”.
'forEach' is only for reading

[urugator]

It doesn't matter which method you use, you shouldn't modify the array (more specifically it's length) during the iteration, because then you're risking that you will skip or re-do some elements, depending on how the iteration protocol is implemented. Some languages even throw an exception is such situations: https://docs.oracle.com/javase/7/docs/api/java/util/ConcurrentModificationException.html
I will reopen this as I am still investigating whether that arr bussiness is intended/expected. Since it deviates from standard array behavior it should be at least mentioned in docs.

[urugator]

Confirming this is a bug: The 3rd argument passed to forEach callback should be the observable proxy instead of the internal proxied array. Repro: #2326 (comment)
Other functions taking callback as an arg can be affected as well.
PR welcome.