[iOS] removedChildren count (0) was not what we expected (1)

[msand]

🐛 Bug Report

RCTUIManager throws exceptions when managing children where a parent has both static (some without shadowViews) and dynamic content.

If the children contain any Defs component (which is/was returning nil for the shadowView in it's ViewManager) and the dynamic part changes size/number of elements, then the indices of the children do not match up.

Several similar issues have been opened in react-native-svg, and e.g. in several charting libraries built on top of it etc. and various workarounds exist, but the problem keeps popping up and should probably just work.

https://github.com/FormidableLabs/victory-native/issues/432
indiespirit/react-native-chart-kit#62
msand/react-native-svg-charts@e8b0baa
JesperLekland/react-native-svg-charts#280
JesperLekland/react-native-svg-charts#244
software-mansion/react-native-svg#258
software-mansion/react-native-svg#848

To Reproduce

git clone https://github.com/msand/ReproRCTUIManagerBug.git
cd ReproRCTUIManagerBug
yarn
react-native run-ios

Or do the following steps (using react-native-svg v7.0.0 - v.9.2.3, it's fixed in v9.2.4 and mostly didn't exist in v6.5.2 and earlier)

react-native init
yarn add [email protected] victory-native
react-native link
change App.js to reproduction code
react-native run-ios

Click either Increment or Decrement button

Expected Behavior

The chart renders without exceptions, and re-renders content according to the state.

Code Example

https://snack.expo.io/@msand/groaning-chocolate
https://github.com/msand/ReproRCTUIManagerBug
The code works in expo, but not with latest react-native.
Update: Doesn't work in Expo either, it just doesn't throw any exception, and does a partial re-render. If you click decrement twice, then the indices will align, and it'll start re-rendering with new content. At that point, incrementing will work as well.

Environment

React Native Environment Info:
System:
OS: macOS High Sierra 10.13.6
CPU: (4) x64 Intel(R) Core(TM) i5-4278U CPU @ 2.60GHz
Memory: 109.55 MB / 8.00 GB
Shell: 3.2.57 - /bin/bash
Binaries:
Node: 11.2.0 - /usr/local/bin/node
Yarn: 1.12.3 - /usr/local/bin/yarn
npm: 6.7.0 - /usr/local/bin/npm
Watchman: 4.9.0 - /usr/local/bin/watchman
SDKs:
iOS SDK:
Platforms: iOS 12.1, macOS 10.14, tvOS 12.1, watchOS 5.1
Android SDK:
API Levels: 16, 23, 24, 25, 26, 27, 28
Build Tools: 23.0.1, 25.0.0, 25.0.1, 25.0.2, 25.0.3, 26.0.1, 26.0.2, 26.0.3, 27.0.1, 27.0.2, 27.0.3, 28.0.2, 28.0.3
System Images: android-16 | Google APIs Intel x86 Atom, android-26 | Google APIs Intel x86 Atom_64, android-27 | Google APIs Intel x86 Atom, android-28 | Google APIs Intel x86 Atom
IDEs:
Android Studio: 3.3 AI-182.5107.16.33.5199772
Xcode: 10.1/10B61 - /usr/bin/xcodebuild
npmPackages:
react: 16.6.3 => 16.6.3
react-native: 0.58.4 => 0.58.4
npmGlobalPackages:
create-react-native-app: 2.0.2
react-native-cli: 2.0.1
react-native-create-library: 3.1.2
react-native-git-upgrade: 0.2.7
react-native-init: 0.6.7

Workaround

Change

    return React.cloneElement(groupComponent, groupProps, [
      clipComponent,
      ...React.Children.toArray(children),
    ]);

To

    return React.cloneElement(groupComponent, groupProps, [
      ...React.Children.toArray(children),
      clipComponent,
    ]);

This places the component which is missing the shadowView after the changing content. In some other cases, it requires wrapping the part with changing number of children in another parent, e.g. using a G element (used for grouping svg elements) to isolate it from the missing shadowView, i.e. Defs elements, causing the indices to misalign. Or, to isolate the Defs elements using a G element.

Exceptions

Exception when decrementing:
removedChildren count (0) was not what we expected (1)

Exception when incrementing

Exception '*** -[__NSArrayM insertObject:atIndex:]: index 4 beyond bounds [0 .. 2]' was thrown while invoking manageChildren on target UIManager with params (
    55,
        (
    ),
        (
    ),
        (
        327
    ),
        (
        4
    ),
        (
    )
)
callstack: (
	0   CoreFoundation                      0x00000001129121bb __exceptionPreprocess + 331
	1   libobjc.A.dylib                     0x0000000110ff7735 objc_exception_throw + 48
	2   CoreFoundation                      0x000000011285e4ec _CFThrowFormattedException + 194
	3   CoreFoundation                      0x000000011283a794 -[__NSArrayM insertObject:atIndex:] + 1300
	4   SvgExample                          0x000000010f50357f -[RCTShadowView insertReactSubview:atIndex:] + 399
	5   SvgExample                          0x000000010f52d996 -[RCTUIManager _manageChildren:moveFromIndices:moveToIndices:addChildReactTags:addAtIndices:removeAtIndices:registry:] + 2966
	6   SvgExample                          0x000000010f52c8f6 -[RCTUIManager manageChildren:moveFromIndices:moveToIndices:addChildReactTags:addAtIndices:removeAtIndices:] + 342
	7   CoreFoundation                      0x000000011291903c __invoking___ + 140
...

[msand]

Seems another workaround is to make the ViewManager return nil for the shadowView: software-mansion/react-native-svg@9e516a2
But, I suspect this will affect (native) animations: software-mansion/react-native-svg#755
@shergin @hramos Any advice?

[msand]

So I've found the cause and fixed it for react-native-svg and friends at least, and updated the issue description with my conclusions. Released v9.2.4

The issue was that another ViewManager (RNSVGDefsManager) was still returning nil, thus causing the indices to misalign. Making all the ViewManagers use the shadowView method inherited from the RCTViewManager allows keeping the animation support as is.

I wonder, should RCTViewManager / RCTUIManager take into account if the shadowView is nil for some children/siblings, and adjust the indices accordingly? Instead of throwing exceptions.

Or are there expectations/contracts on the ViewManager never to return nil? The comments say:

should return a fresh instance each time it is called

But, it also works fine if all the ViewManagers return nil. So, not very expected that it would crash when combining components, having both types of ViewManagers / shadowView methods, inside the same parent.

[msand]

Made a reproduction which is as small as I could imagine:

import React, { Component } from 'react';
import { View, Text, StyleSheet, TouchableOpacity } from 'react-native';
import { Svg, Defs, Circle } from 'react-native-svg';

const colors = ['black', 'red', 'green', 'blue'];

export default class App extends Component {
  state = { length: 3 };
  inc = () => {
    this.setState(({ length }) => {
      return { length: length + 1 };
    });
  };
  dec = () => {
    this.setState(({ length }) => {
      return { length: length - 1 };
    });
  };
  render() {
    return (
      <View style={style.container}>
        <View style={style.buttons}>
          <TouchableOpacity onPress={this.inc} style={style.button}>
            <Text>Increment</Text>
          </TouchableOpacity>
          <TouchableOpacity onPress={this.dec} style={style.button}>
            <Text>Decrement</Text>
          </TouchableOpacity>
        </View>
        <Svg width="100%" height="100%" viewBox="0 0 100 100">
          <Defs />
          {Array.from(this.state).map((_, i) => (
            <Circle
              key={i}
              x="50"
              y="50"
              r={50 / (i + 1)}
              fill={colors[i % colors.length]}
            />
          ))}
        </Svg>
      </View>
    );
  }
}
const style = StyleSheet.create({
  container: { backgroundColor: 'white', paddingTop: 40 },
  buttons: { flexDirection: 'row' },
  button: {
    margin: 10,
    padding: 10,
    borderWidth: 1,
    borderColor: 'black',
  },
});

The Defs element returns nil for the shadowView in v7.0.0-v9.2.3 of react-native-svg, making the indices in manageChildren go haywire when changing the number of Circle elements, which do have a RCTShadowView as the shadowView. The same goes for every other kind of element in react-native-svg (excluding only the Defs element).

[msand]

The easiest workaround is probably to wrap/isolate the Defs element(s) in a G element for now, if one needs backwards compatibility with pre v9.2.4, and/or wrapping any changing number of elements in a G element. But, upgrading to the latest version, and only using G elements where it is useful for structure or performance would be the best.

[stale]

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as a "Discussion" or add it to the "Backlog" and I will leave it open. Thank you for your contributions.

[stale]

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please feel free to create a new issue with up-to-date information.